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

PROPERTY_ACCESS_ERROR with Google AutoValue #548

Closed
jonfreedman opened this issue Jun 19, 2017 · 15 comments
Closed

PROPERTY_ACCESS_ERROR with Google AutoValue #548

jonfreedman opened this issue Jun 19, 2017 · 15 comments
Labels

Comments

@jonfreedman
Copy link
Contributor

@jonfreedman jonfreedman commented Jun 19, 2017

Google have a nice library AutoValue to generate immutable object model classes. To use this you create a public abstract class with abstract methods for each object property and at compile time an implementation is generated with package class access.

There are several issues when trying to use this library:

  • The Id attribute is not picked up without switching to MappingStyle.BEAN

  • The Id attribute is not picked up on an abstract method

  • Due to the way properties are looked up using reflection this results in IllegalAccessExceptions being throw from org.javers.common.reflection.JaversGetter. It would be possible to avoid this & potentially other polymorphic issues by traversing up the class hierarchy until you find a public class on which to perform reflection.

I have created a failing test here which fails with the InvocationTargetException.
If you move the @Id attribute from Animal#getJaversId to Animal#getName the tests fail with an ENTITY_WITHOUT_ID error.
If you remove .withMappingStyle(MappingStyle.BEAN).registerEntity(Animal.class) then the test will fail with id = 1.0 not Cat.

I would be happy to help fix this with some pointers.

@jonfreedman
Copy link
Contributor Author

@jonfreedman jonfreedman commented Jun 19, 2017

I see that abstract methods are ignored in JaversGetterFactory - removing that requirement allows Animal#getName to be annotated with @Id.

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Jun 19, 2017

Thanks for reporting. Well, we can do changes but we cant add explicit dependency on this google lib to javers-core. So is it possible to reword this issue in a more abstract way?

@jonfreedman
Copy link
Contributor Author

@jonfreedman jonfreedman commented Jun 19, 2017

I added the dependency to the test-scope - I expect you could illustrate the issue with a non-abstract class that wasn't auto-generated. I'm just attempting to dig into this further.

@jonfreedman
Copy link
Contributor Author

@jonfreedman jonfreedman commented Jun 19, 2017

I have a fix for this, not sure if it's sensible, basically allowing abstract methods and for each iteration of the while loop in JaversGetterFactory#getAllGetters check if the new methods we've found override existing ones and remove those which have been overridden. Trouble is that this causes the TypeFactoryBeanIdTest test case to fail because the @Id annotation is on the subclass but not the superclass. I've committed those changes to my fork, it's definitely possible to make that test pass but I think the solution would be eagerly deciding if a JaversMember "looksLikeId" and reading that in Property#looksLikeId as that "id-ness" would need to be passed up the inheritance hierarchy when removing from getters in JaversGetterFactory.

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Jun 19, 2017

The issue is still unclear to me, so you are saying that when @id annotation is on abstract method in a superclass and concrete method is in a subclass, Javers will break?

This simple test shows that this is not a problem for JaVers. So can you please write a failing test which will show the issue but without code generation?

class AutoValueCase extends Specification{

    abstract class AbstractEntity {
        @Id
        abstract int getId()
    }

    class ConcreteEntity extends AbstractEntity {
        int value
        int id

        @Override
        int getId() {
            id
        }
    }

    def "should support abstract idGetter"(){
      given:
      def javers = JaversBuilder.javers().build()

      when:
      def a = new ConcreteEntity(id:1, value: 1)
      def b = new ConcreteEntity(id:2, value: 2)

      then:
      def diff = javers.compare(a,b)
      diff.changes[0].left == 1
      diff.changes[0].right == 2
    }
}
@jonfreedman
Copy link
Contributor Author

@jonfreedman jonfreedman commented Jun 20, 2017

Sure, I've moved the JUnit test to Spock, I'm sure the test can be improved, especially how the @Id annotation is checked on line 61. What I'm trying to show is that if you use MappingStyle.FIELD you don't get ids, and if you switch to MappingStyle.BEAN without my changes you get exceptions.

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Jun 20, 2017

ok, now I see the problem, when @Id is on abstract getter, it's ignored. This test fails on master:

class AutoValueCase extends Specification {

    abstract class AbstractEntity {
        @Id
        abstract int getId()

        abstract int getValue()
    }

    class ConcreteEntity extends AbstractEntity {
        int id
        int value

        @Override
        int getId() {
            id
        }

        @Override
        int getValue() {
            value
        }
    }

    @Unroll
    def "should map #entity.simpleName with abstract @IdGetter as EntityType"(){
      given:
      def javers = JaversBuilder.javers().withMappingStyle(MappingStyle.BEAN).build()

      expect:
      javers.getTypeMapping(entity) instanceof EntityType

      where:
      entity << [AbstractEntity, ConcreteEntity]
    }
}
@jonfreedman
Copy link
Contributor Author

@jonfreedman jonfreedman commented Jun 20, 2017

Great, there's also an accessibility issue if you make ConcreteEntity @PackageScope (https://github.com/javers/javers/pull/549/files#diff-47041ce08130c535bf8ab0714c2bcdbbR19)

@jonfreedman
Copy link
Contributor Author

@jonfreedman jonfreedman commented Jun 20, 2017

FYI your failing test passes on my fork

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Jun 20, 2017

ok, one thing at a time, let's manage abstract getter first. I have a few open issues now, so please be patient

@jonfreedman
Copy link
Contributor Author

@jonfreedman jonfreedman commented Jun 20, 2017

I can leave it with you but I have fixed both the issues, and I need them both resolved to work with AutoValue.

Cheers

Jon

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Jun 20, 2017

just a small survey: we are going to start enterprise services, with an option of buying features or bugfixes with fast delivery.

Do you think that your company would be eager to pay for having this issue delivered in this week?

@bartoszwalacik bartoszwalacik added enhancement and removed bug labels Jun 20, 2017
@jonfreedman
Copy link
Contributor Author

@jonfreedman jonfreedman commented Jun 20, 2017

It would depend on how much it cost plus for an ongoing license we would expect a relevant product roadmap of new features. We're more likely to contribute a PR and fork a fixed version internally if there's an issue with release cycles.

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Jun 20, 2017

The license stays as is - open source (Apache License). You would get only faster release of a requested feature.

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Jun 29, 2017

fixed & released in 3.3.3

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

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.