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

Avoid the unnecessary UPDATE for JsonNode entity mappings #348

Closed
victornoel opened this issue Sep 28, 2021 · 15 comments
Closed

Avoid the unnecessary UPDATE for JsonNode entity mappings #348

victornoel opened this issue Sep 28, 2021 · 15 comments
Labels
Milestone

Comments

@victornoel
Copy link

victornoel commented Sep 28, 2021

Hi,

I'm hitting a strange bug using hibernate-types 2.12.0 and hibernate 5.4.32 and I'm still investigating it, but thought I could already raise it here in case someone had an idea about it :)

Basically, I have one class:

public class MyJsonType extends com.vladmihalcea.hibernate.type.json.JsonType {
    public MyJsonType() {
        super(MyApp.getMyJsonMapper()); // configured with DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS
    }
}

I declared the json type with:

@TypeDef(name = "json", typeClass = MyJsonType.class)

I'm using MariaDB103Dialect and I also register the following:

registerColumnType(Types.OTHER, "json");

I also use the bytecode enhancement for dirty checking.

And I have a class with a field:

    @JsonProperty
    @Basic
    @Type(type = "json")
    private JsonNode state;

And then I persisted such an object with state with value {"field":0.05}.

When I retrieve it with the hibernate session, every time there is a flush, the dirty checking mechanism is triggered and tells me the value of state changed even though it wasn't, and an update statement is issued to the db (which include updating the version field of my object, which I don't want :)

@vladmihalcea
Copy link
Owner

vladmihalcea commented Sep 28, 2021

@victornoel Check out this test case that validates the number of statements that get generated.

You can use that as a template for a new test case that uses your mapping and replicates the issue. If you can replicate it, send me a Pull Request so I can take a look at it. Looking forward to it.

victornoel pushed a commit to victornoel/hibernate-types that referenced this issue Sep 28, 2021
@victornoel
Copy link
Author

@vladmihalcea thank you, here it is: #349

@victornoel
Copy link
Author

victornoel commented Sep 28, 2021

@vladmihalcea a few related issues at jackson that are related to this (and could be a good justification for considering this not a bug in hibernate-types ;)):

By the way the workaround proposed there does not solve the problem, so maybe not related ^^

@vladmihalcea
Copy link
Owner

@victornoel Thanks for investigating it. Indeed, using String should fix the problem or using a POJO for the JSON object as you can define your own equals and hashCode methods.

@victornoel
Copy link
Author

@vladmihalcea using Map (since I know it's a json dict) instead of JsonNode also works as a workaround. This is good enough for my use case here :)

I looked a bit around the various bugs in Jackson and also did some tests, and I'm still not clear why it doesn't work as it should, because if I just deserialize my json as JsonNode and try to test equals on it, it seems to work as expected :/

But I'm not familiar enough with hibernate-types to clearly understand what is used to do that dirty check in practice, so I suppose you will be more efficient than me to understand what's happening ^^

@vladmihalcea
Copy link
Owner

If only I had the time to investigate it. I usually investigate issues that are either critical or there's a company who wants to sponsor the fix.

I'll let the issue open for a while to see if anyone wants to provide a good fix for this.

@victornoel
Copy link
Author

Ok, I will see if I can investigate it a bit when I get some time then :)

Thx for your guidance to reproduce it with simple setup in any case!

@vladmihalcea
Copy link
Owner

@victornoel You're welcome. Looking forward to reviewing your fix.

@victornoel
Copy link
Author

@vladmihalcea so, first results of investigation:

  • when we use DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS, a json float number is parsed to a DecimalNode object (which wraps a BigDecimal), not a DoubleNode (which wraps a simple double)
  • so our properties in the test repro is a JsonNode that contains a DecimalNode
  • when the entity is loaded in the test repro, at one point, during TwoPhaseLoad.initializeEntityFromEntityEntryLoadedState that is responsible of storing the loaded state (so that it can be compared to the current state during dirty checking), hibernate clones the properties
  • it uses hibernate-types's ObjectMapperJsonSerializer.clone, which delegates to hibernate-types's SerializationHelper.clone, which does not use the ObjectMapper!
  • and so the resulting clone, is using java deserialization that seems to produce a DoubleNode for json float number.

Next I will try to see what could be the best way to improve this.

I guess there are good reasons to rely on java deserialization by default?

I know that JsonNode do have a deepCopy() method for example that could be used here.

I also saw that it is possible to implement a custom JsonSerializer so this could be a way to workaround that bug too :)

Thanks!

victornoel pushed a commit to victornoel/hibernate-types that referenced this issue Sep 29, 2021
@victornoel
Copy link
Author

(I have pushed a first naive fix just to demonstrate it :)

victornoel pushed a commit to victornoel/hibernate-types that referenced this issue Sep 30, 2021
@victornoel
Copy link
Author

victornoel commented Oct 6, 2021

I have also opened FasterXML/jackson-databind#3296 but:

  • I'm not sure it's going to be fixed since there are a lot of BigDecimal related problems in Jackson that may take some time to be tackled
  • I believe the PR I made is still interesting because it should improve reliability as well as performance :)

@vladmihalcea
Copy link
Owner

@victornoel Thanks. I'll investigate it when I have some spare time. It's just that I'm very busy at the moment.

@victornoel
Copy link
Author

victornoel commented Oct 6, 2021

@vladmihalcea no worries, whenever you can, this is not urgent nor blocking for me :) (actually, even if it was :P)

@vladmihalcea vladmihalcea changed the title Dirty check triggered while reading a JsonNode from db Avoid the unnecessary UPDATE for JsonNode entity mappings Oct 9, 2021
@vladmihalcea vladmihalcea added this to the 2.13.0 milestone Oct 9, 2021
@vladmihalcea
Copy link
Owner

Fixed.

@victornoel
Copy link
Author

@vladmihalcea thx :)

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

No branches or pull requests

2 participants