Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for JSON bean mutation detection via HASH and SOURCE of JSON string content #2274

Merged
merged 27 commits into from
Jul 30, 2021

Conversation

rbygrave
Copy link
Member

@rbygrave rbygrave commented Jul 23, 2021

  • Only converts to JSON once (at dirty detection time)

On load:

  • For HASH mode, only compute CRC32 checksum once, checksum stored on EntityBeanIntercept for mutation detection
  • For SOURCE mode, store original json content on EntityBeanIntercept for mutation detection

SOURCE mode supports regenerating the 'oldValue' for change log and persist listeners. It does this by rebuilding the 'oldValue' from the original json source that was stored on EntityBeanIntercept.

Both SOURCE and HASH support BeanState.isDirty() checking.

- MD5 of json content stored on EntityBeanIntercept for dirty detection
- Only convert to JSON once (at dirty detection time). Store this json content on EntityBeanIntercept to later push to ScalarTypeJsonObjectMapper for bind

Should consider alternative to extend BeanProperty rather than have these if blocks.
@rbygrave
Copy link
Member Author

@rPraml ... maybe have a look at this and see what you think.

Cheers, Rob.

@rPraml
Copy link
Contributor

rPraml commented Jul 24, 2021

Hello Rob, I did a very quick (= 5 minutes) look at this. (Will take a detailed look at this next week)
Dirty detection should work this way, but I wonder if it makes sense, to store the MD5 hash instead of the whole json string.
(Tradeoff between RAM and CP'U time)

Note, we have the second use case, that we need the originalValue / valuePair for tracking the changes in a changelog.
If the whole string would be stored, it would/should be possible to reconstruct the originalValue when BeanPersistRequest.getUpdatedValue / beanState.getChangedValues is called.

Cheers
Roland

@rbygrave
Copy link
Member Author

Yes @rPraml we can probably work out how to make that an option. With this current PR I think we can try to move some of the logic into the ScalarType (from BeanProperty) so I think there are a couple of adjustments to this.

rbygrave and others added 21 commits July 28, 2021 21:43
Note that we don't need to use readSetIntercept() now as there is no property change listener support
…_list_loadedAndNotDirtyAware()

    BeanState state = DB.getBeanState(found);
    assertThat(state.getChangedProps()).containsExactlyInAnyOrder("beanList");
    // this test fails, because we have a OmList instead of a GenericObject
    // TODO: Can/Should we enhance the @DbJson/@DbJsonB annotations with a property "dirtyDetection"
Remove scalarType.jsonMapper(value) as we can just use format(value) instead
Plus adjust timing on SqlQueryCancelTest
- Bump ebean-annotation with new @dbjson dirtyDetection and keepSource attributes
- Get those to BeanPropertyJsonMapper to chose MutableHash implementation
- Modify MD5MutableHash to include check for isDirty()
- EBasicJsonList needs keepSource=true to pass that test with oldValue
… is only done once

Adds MutableValueInfo.nextDirty() to replace the isEqualToJson() method. The next is computed once and stored. BindablePropertyJsonUpdate makes the .mutableNext(propertyIndex) call to move the next MutableValueInfo and return the json content.
Also adds NoMutationDetection to support NONE
…beanList property

  @DbJsonB(mutationDetection = HASH)
  List<PlainBean> beanList;
…known dirty go into beanProperty.checkMutable()

As per rPraml's PR and comment

Due to handling of mutableNext we need  checkMutableProperties() to call into what is now beanProperty.checkMutable() even when we already know it's dirty.
@rbygrave
Copy link
Member Author

Ok @rPraml I think this is pretty close. Did you want to add any more test cases?

@rbygrave rbygrave changed the title JSON bean dirty detection via MD5 of JSON string content Add support for JSON bean mutation detection via HASH and SOURCE of JSON string content Jul 29, 2021
@rbygrave rbygrave self-assigned this Jul 29, 2021
@rPraml
Copy link
Contributor

rPraml commented Jul 30, 2021

Hello @rbygrave I think the current solution covers all of our use cases and is ready to merge from my point of view.
Maybe you can check if #2248 / #2250 is obsolete now.
Thanks for your support

@rbygrave
Copy link
Member Author

if #2248 / #2250 is obsolete now.

Yes, those are obsolete now @rPraml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants