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

Potential bug due to the non-determinicity of Class#getDeclaredConstructors #682

Closed
Oja95 opened this Issue Jun 21, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@Oja95

Oja95 commented Jun 21, 2018

During initializing the modules, reflection is used to instantiate various classes needed for the modules.
However, the utility responsible for instantiating these classes takes the first public constructor from the array returned by Class#getDeclaredConstructors method and uses it to instantiate the class. See org.javers.common.reflection.ReflectionUtil#newInstance.

However, the Javadoc from Class#getDeclaredConstructors states the following:

The elements in the array returned are not sorted and are not in any particular order.

Thus, relying on the order of the constructors obtained might result in unexpected behaviour.

A real erroneous scenario occurred during initializing the InMemoryRepositoryModule, more specifically, during instantiating the InMemoryRepository class. InMemoryRepository has two public constructors:

public InMemoryRepository()
public InMemoryRepository(CommitIdGenerator commitIdGenerator)

Should the Class#getDeclaredConstructors return an array in which the default public constructor is the first element, everything works well. However, should the second constructor be the first element, then it fails to resolve the CommitIdGenerator object reference from the Pico container. The following is then logged:

ERROR org.javers.core.pico.ContainerArgumentResolver - failed to get component of type org.javers.core.CommitIdGenerator from Pico container
ERROR org.javers.common.reflection.ReflectionUtil - failed to create new instance of org.javers.repository.inmemory.InMemoryRepository, argument resolver for arg[0] org.javers.core.CommitIdGenerator thrown exception: COMPONENT_NOT_FOUND: JaVers bootstrap error - component of type 'org.javers.core.CommitIdGenerator' not found in container

Steps to reproduce

There is no good way to reliably reproduce this by some simple application. To reproduce the issue, create a simple main method that builds a default Javers instance using the following:

JaversBuilder.javers().build();

Then add a conditional breakpoint to line 70 in org.javers.common.reflection.ReflectionUtil to only break when clazz == InMemoryRepository.class holds. Run the application, and when the application stops on the breakpoint, check the constructor variable to see if it references the default constructor. If so, evaluate an expression to change the value of the constructor variable to become the other constructor obtained by Class#getDeclaredConstructors using the following expression:

constructor = clazz.getDeclaredConstructors()[1];

After that, resume the execution and the mentioned errors should pop right up in the standard output. Please keep in mind that this difficult hack to reproduce the issue is to simulate a potential scenario that could very well happen due to the non-determinicity of the order of elements returned by Class#getDeclaredConstructors method.

Reality and expectation

The org.javers.common.reflection.ReflectionUtil#newInstance instantiation method has non-determinicity in it which might cause unexpected behaviour. Expectation would be to ensure that the initialization logic wouldn't break because of the non-determinicity of the order of the elements of Class#getDeclaredConstructors method.

Oja95 added a commit to Oja95/javers that referenced this issue Jun 21, 2018

bartoszwalacik added a commit that referenced this issue Jul 7, 2018

bartoszwalacik added a commit that referenced this issue Jul 7, 2018

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jul 7, 2018

@Oja95 thanks for your help in finding and fixing this bug.
I decided to fix it in a different way, see https://github.com/javers/javers/pull/686/files, so I going to close your PR. InMemoryRepository instance is now created directly in JaversBuilder, without relying on constructor/reflection magic.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jul 7, 2018

fix is on its way to the Central
release javers-3.10.1

@bartoszwalacik bartoszwalacik added the fixed label Jul 7, 2018

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jul 7, 2018

fix released in javers-3.10.1

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