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

Javers fails when configured using ClientsClassDefinition with includedProperties #650

Closed
stefanicai opened this Issue Mar 11, 2018 · 11 comments

Comments

Projects
None yet
2 participants
@stefanicai
Contributor

stefanicai commented Mar 11, 2018

Test:

def "Javers should be able to build when a class is configured using ClientsClassConfiguration with includedProperties"() {
    when:
    def javers = JaversBuilder.javers().registerEntity(entityDefinition).build()
    javers.commit("user", entityInstance)

    then:
    noExceptionThrown()

    where:
    entityDefinition << [EntityDefinitionBuilder.entityDefinition(DummyUser.class).withIdPropertyName("name").withIncludedProperties(["name", "surname"]).build()]
    entityInstance << [new DummyUser("Johny", "Doe").withAddress("Melbourne")]
}

I've added the test in branch Fix_issues_with_registering_clientsclassdefinition_with_included_Properties

@stefanicai

This comment has been minimized.

Contributor

stefanicai commented Mar 11, 2018

Just switch to that branch and test IncludedPropertiesTest

@stefanicai

This comment has been minimized.

Contributor

stefanicai commented Mar 12, 2018

The issue is that we handle Transient and DiffIgnore the same. So it's likely that we'll have some Transient props in many classes, including some that we choose to use DiffInclude. We can either handle transient and ignore separate (so a field would have hasDiffIgnoreAnn and hasTransientAnn, instead of just the later), or we consider that when the user uses DiffInclude, they take over and we ignore all the ignore annotations.

I'd go for the second option, if the user uses DiffInclude annotation, we ignore the DiffIgnore or Transient (and maybe just log a message to say so, for debugging purposes). The user should basically know what they are doing in that case.

@bartoszwalacik , what do you think?

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Mar 13, 2018

Ok, so it's not a bug, but simply a conflict between Transient (which is the synonym to DiffIgnore) and DiffInclude.
Why do you think that DiffInclude should force ignoring all DiffIgnores? It would be a side effect. Now, both annotations have the same rights, so you just can't use both. The design is clear.

@stefanicai

This comment has been minimized.

Contributor

stefanicai commented Mar 13, 2018

The conflict comes from the fact that Transient is not a Javers annotation. It's a JPA annotation.
It is very likely that we'd have a model that has some properties that we don't want to save in the Database, but we'd like to describe that model using DiffInclude or ClientsClassDefinition with includedProperties. In that case both includedProperties and ignoredProperties(which include Transient props) are non empty. Thus the condition of only one being provided fails. Note, in this case no DiffIgnore annotation was used. I think a scenario like this should be accepted, otherwise the use of DiffInclude is quite restricted to only super simple models, which is not realistic in more serious applications.

Does it make sense?

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Mar 14, 2018

Transient is treated by JaVers as synonym to DiffIgnore. Also, there is one to one mapping between other jpa annotations and javers annotations. They are synonyms. This design rule has been in Javers from the very beginning and it still makes sense so I'm not going to break it

@stefanicai

This comment has been minimized.

Contributor

stefanicai commented Mar 14, 2018

@bartoszwalacik that limits the usage of DiffInclude a lot. Say we have this class:

class Person {
    @Id
    Long id;
    String firstName;
    String lastName;

   //lots of other field here, say 20 more.

    @Transient
    String getFullName() { return firstName + " " + lastName; }
 }

That class works well as it goes. Transient will be read as DiffIgnore and that property is not going to be logged/compared. Now, if we want to say that we only want to log the fields id, firstName and lastName from that class and we'd like to add @DiffInclude to those fields and not worry about the other 20 fields. That's nicer than having to annotate with DiffIgnore all those other 20 fields and is the reason why we introduced DiffInclude.

Well, that will fail with an exception in ManagedPropertiesFilter

ManagedPropertiesFilter(Class<?> baseJavaClass, List<JaversProperty> allSourceProperties, PropertiesFilter propertiesFilter) {

    this.includedProperties = filter(allSourceProperties, propertiesFilter.getIncludedProperties(), baseJavaClass);
    this.includedProperties.addAll(allSourceProperties.stream().filter(p -> p.isHasIncludedAnn()).collect(Collectors.toSet()));

    this.ignoredProperties = filter(allSourceProperties, propertiesFilter.getIgnoredProperties(), baseJavaClass);
    this.ignoredProperties.addAll(allSourceProperties.stream().filter(p -> p.hasTransientAnn()).collect(Collectors.toSet()));

     //*******THIS WILL ALWAYS FAIL********
    //Due to that @Transient annotation which Javers sees the same as @DiffIgnore
    //Because of that, we'll now have one ignoredProperty and 3 includedProperties.

    if (this.ignoredProperties.size() > 0  && this.includedProperties.size() > 0) {
        throw new JaversException(JaversExceptionCode.IGNORED_AND_INCLUDED_PROPERTIES_MIX, baseJavaClass.getSimpleName());
    }
}

I don't think that is what we want. We want the user to be able to use @DiffInclude even for classes where they have transient properties.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Mar 14, 2018

we can talk about changing the behavior of DiffInclude, maybe it could coexists with DiffIgnore, but DiffIgnore have to be synonym to Transient

@stefanicai

This comment has been minimized.

Contributor

stefanicai commented Mar 14, 2018

Sure, that's not what I'm suggesting. Was just saying that's the reason why this happens, because those two are treated the same.

Some options of how we could potentially fix this:

  1. When DiffInclude is present, we ignore all DiffIgnore or Transient properties and ONLY log DiffInclude props. We can log.debug that so users can debug if it's confusing, though I don't see why it should be. As I see it, that's the point of DiffInclude, to enable you to say that ONLY some fields are to be considered by JaVers, and everything else ignored.
  2. We check in PropertyScanner if we have for that class both DiffIgnore and DiffInclude and throw an exception already there. We don't take into account Transient when doing this check. The rest of the code stays the same (aka the method hasTransientAnn will be set to true for both DiffIgnore and Transient as it is today).
  3. We have two properties in JaversProperty: hasTransientAnn and hasDiffIgnore and create another method shouldBeIgnored() which returns hasTransientAnn || hasDiffIgnore, which we then use everywhere hasTransientAnn is used today.
@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Mar 14, 2018

Option no 1 looks best because I dont want the distinction between DiffIgnore and Transient

@stefanicai

This comment has been minimized.

Contributor

stefanicai commented Mar 14, 2018

ok, I'll send a PR shortly for that.

@stefanicai stefanicai reopened this Mar 14, 2018

stefanicai pushed a commit that referenced this issue Jun 12, 2018

Iulian Stefanica
#650 fixed conflict by not taking into account @DiffIgnore and @trans…
…ient when @DiffInclude is present. Added another test and fixed the old one.

bartoszwalacik added a commit that referenced this issue Jun 22, 2018

Merge pull request #681 from javers/#650_Fix_conflicts_when_both_Diff…
…Include_and_DiffIgnore_present

#650 fixed conflict by not taking into account @DiffIgnore and @trans
@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jun 23, 2018

released in 3.10.0

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