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

Specify ignored properties without annotations #94

Closed
ctmay4 opened this Issue Feb 1, 2015 · 10 comments

Comments

Projects
None yet
3 participants
@ctmay4

ctmay4 commented Feb 1, 2015

Currently, you can ignore properties for comparisons using the @DiffIgnore annotation. There are times when you want to track history for entities which you do not have the ability to annotate. It would be very helpful if you can programmatically give a list of properties to ignore when committing changes.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Feb 1, 2015

I can see two ways for solving this problem.

Lets say we have an Entity

public class MyEntity {
  ...
  private NotInterestingType someProperty;
}

One way is to register a type as ignored. All properties with that type in all classes would be ignored:

javers().registerIgnoredType(NotInterestingType.class).build();

Second way (fine grained), register concrete property as ignored:

javers().registerIgnoredProperty(MyEntity.class, "someProperty").build();

Second approach gives you full control but more configuration. I think that first approach would be better (less and type safe configuration)

@ctmay4

This comment has been minimized.

ctmay4 commented Feb 1, 2015

I think I would argue the second solution is superior. What if my class had 2 properties that returned the same entity. One I want to track, the other I did not. Your first solution would not solve that while the second one would.

bartoszwalacik added a commit that referenced this issue Feb 8, 2015

#94
ignoring properties

bartoszwalacik added a commit that referenced this issue Feb 8, 2015

#94
IgnoringPropertiesWithoutAnnTest
@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Feb 8, 2015

Hi Chuck,
check 1.0.6-SNAPSHOT (from https://oss.sonatype.org/content/repositories/snapshots/)

usage example (Spock):

    def "should ignore properties"() {
        given:
        def javers = JaversBuilder.javers().registerEntity(DummyUser, "name", ["surname"]).build()

        when:
        def user1 = new DummyUser("Lord", "Smith")
        def user2 = new DummyUser("Lord", "Garmadon")

        def diff = javers.compare(user1, user2)

        then:
        ! diff.changes
    }
@ctmay4

This comment has been minimized.

ctmay4 commented Feb 8, 2015

Great. I'll run some tests this week.

bartoszwalacik added a commit that referenced this issue Feb 8, 2015

#94
javadoc options.noTimestamp()

bartoszwalacik added a commit that referenced this issue Feb 8, 2015

#94
minor javadoc fixes
@ctmay4

This comment has been minimized.

ctmay4 commented Feb 9, 2015

I just ran some tests are everything looks good from my end. One suggestion I would have is that there are currently 2 versions of registerEntity:

public JaversBuilder registerEntity(Class<?> entityClass);
public JaversBuilder registerEntity(Class<?> entityClass, String idPropertyName, List<String> ignoredProperties);

If I want to register an entity but don't care about ignored properties, I have to pass null as the third parameter. Seems like an easy call through to add

public JaversBuilder registerEntity(Class<?> entityClass, String idPropertyName);

That is a very minor nitpick and it works fine as it is.

I did come across an issue/question about comparisons, but since this issue seems to be complete, I will option another issue for that. Thanks again.

bartoszwalacik added a commit that referenced this issue Feb 9, 2015

#94
CR fixes

bartoszwalacik added a commit that referenced this issue Feb 9, 2015

Merge pull request #96 from javers/#94_ignoring_things
#94 Specify ignored properties without annotations
@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Feb 9, 2015

Interesting, we had similar doubts on Code Review, You are right, passing null as an argument is not nice.

I've changed the API to:

 public JaversBuilder registerEntity(Class<?> entityClass);
 public JaversBuilder registerEntity(EntityDefinition entityDefinition);

and EntityDefinition has constructors for both cases (with and without ignored properties)

I've just pushed new 1.0.6-SNAPSHOT to Sonatype, check it out.
1.0.6 release will be available in one or two days

@ctmay4

This comment has been minimized.

ctmay4 commented Feb 10, 2015

Sounds like a good plan, but the SNAPSHOT has that second registerEntity method as private:

    private JaversBuilder registerEntity(EntityDefinition entityDefinition) {
        clientsClassDefinitions.add(entityDefinition);
        return this;
    }
@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Feb 10, 2015

looks like you have a stale snapshot, nevermind, I've just released 1.0.6 to sonatype, tomorrow morning it will be in maven central

@Endeavourl

This comment has been minimized.

Endeavourl commented Apr 13, 2015

Hi! First of all, thanks for this feature, it really helped!

I found this weird behavior, possibly a bug, when ignoring properties of a super class.

class abstract A {
    private final String irrelevantString;
    private final String string;
}
class B extends A {
    private final String irrelevantString;
}

Ignoring as

new ValueObjectDefinition(B.class, Arrays.asList("irrelevantString"))

works as expected, ignoring the property in the derived class.
However

new ValueObjectDefinition(A.class, Arrays.asList("irrelevantString"))

doesn't work at all.

Another thing, and this is more annoying, is that when there isn't such name conflict between super and derived classes you still cannot ignore super class property.
Instead you have to ignore it for every derived class like this

new ValueObjectDefinition(B.class, Arrays.asList("irrelevantString"))
new ValueObjectDefinition(C.class, Arrays.asList("irrelevantString"))
new ValueObjectDefinition(D.class, Arrays.asList("irrelevantString"))
// etc
// none of B, C or D contain irrelevantString property
// yet using A.class doesn't work

Should i open an issue about this?

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Apr 14, 2015

ok, I will check it, but please post it as a new Issue, we focus on open issues

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