Unable to write custom implementation of CRUD method with generic parameters [DATACMNS-912]

[spring-projects-issues]

Gerard Nantel opened DATACMNS-912 and commented

As discussed in https://jira.spring.io/browse/DATACMNS-854, I am opening a new issue as I am unable to create a custom implementation of a CRUD method that takes generic parameters.

I modified my example that reproduces the problem and removed Mockito from the picture. I also tested the flow in our code base at runtime (not in unit tests), and am also able to reproduce.

Here is the potential DefaultRepositoryInformationUnitTests test that reproduces:

    class BossRepositoryCustom {
        Boss save(Boss boss) {
            return boss;
        }
    }

    @Test
    public void discoversCustomlyImplementedCrudMethodWithGenericParameters() throws SecurityException, NoSuchMethodException {
        BossRepositoryCustom customImplementation = new BossRepositoryCustom();
        RepositoryMetadata metadata = new DefaultRepositoryMetadata(BossRepository.class);
        RepositoryInformation information = new DefaultRepositoryInformation(metadata, CrudRepository.class,
                                                                             customImplementation.getClass());

        Method customBaseRepositoryMethod = BossRepository.class.getMethod("save", Object.class);
        assertThat(information.isCustomMethod(customBaseRepositoryMethod), is(true));
    }

I've also attached a self-contained small test that also reproduces the issue.

Let me know if you need more information. Hopefully I am not missing something obvious here.

---

Affects: 1.12.2 (Hopper SR2)

Attachments:

• DefaultRepositoryInformationUnitTest2.java (1.40 kB)

Issue Links:

• DATACMNS-1008 Custom implementation of CrudRepository.save(…) not called if generics not declared exactly

• DATACMNS-854 Custom implementation of repository fails when overriding methods containing generics

Backported to: 1.12.3 (Hopper SR3), 1.11.5 (Gosling SR5)

[spring-projects-issues]

Oliver Drotbohm commented

You repository setup is incorrect. You have a custom implementation class but no interface. if you introduce that interface and set up your repository as described in the reference documentation, the test succeeds for me.

Are you saying that setup has worked on previous versions?

[spring-projects-issues]

Gerard Nantel commented

Yeah this setup worked up until 1.11.4 inclusive. I like the example in the reference documentation and would much rather do it that way. However, I can't get it to work for the save() method.

I modified my example by adding an interface for the repository customization (with the save() method) and having both the BossRepository interface as well as my BossRepositoryCustom class implement it. However, I can't get my code to compile:

java: reference to save is ambiguous
  both method save(org.springframework.data.repository.core.support.DefaultRepositoryInformationUnitTest2.Boss) in org.springframework.data.repository.core.support.DefaultRepositoryInformationUnitTest2.BossRepositoryCustomInterface and method <S>save(S) in org.springframework.data.repository.CrudRepository match

Then I tried writing a class that implements the BossRepository interface and got:

java: name clash: <S>save(S) and save(org.springframework.data.repository.core.support.DefaultRepositoryInformationUnitTest2.Boss) have the same erasure

Then I changed the signature of my custom method from:

Boss save(Boss boss);

to:

<S extends Boss> S save(S boss);

Now I am able to implement the BossRepository interface. However, I still get a compilation error when trying to call save():

BossRepository repository = new BossRepositoryImpl();
repository.save(new Boss());

java: reference to save is ambiguous
  both method <S>save(S) in org.springframework.data.repository.core.support.DefaultRepositoryInformationUnitTest2.BossRepositoryCustomInterface and method <S>save(S) in org.springframework.data.repository.CrudRepository match

I didn't think it would make a difference but this also does not work:

repository.<Boss>save(new Boss());

If in the BossRepository interface, I override <S extends T> S save(S entity), I do get my call to save() to compile:

...
    repository.save(new Boss());
...
interface BossRepository extends CrudRepository<Boss, Long>, BossRepositoryCustomInterface {
    <S extends Boss> S save(S boss);
}

But I don't think this is the intention and isCustomMethod() still returns false.

Could you show me how you got the test to pass with the repository customization interface? I am currently on the 1.12.x branch by the way

[spring-projects-issues]

Oliver Drotbohm commented

This works without any compiler warning whatsoever:

package org.springframework.data.repository.core.support;

import static org.hamcrest.Matchers.*;
import static org.junit.Assert.*;
import static org.mockito.Mockito.*;

import java.lang.reflect.Method;

import org.junit.Test;
import org.springframework.data.repository.CrudRepository;
import org.springframework.data.repository.core.RepositoryInformation;
import org.springframework.data.repository.core.RepositoryMetadata;

public class DefaultRepositoryInformationUnitTest2 {

	@Test
	public void discoversCustomlyImplementedCrudMethodWithGenericParameters()
			throws SecurityException, NoSuchMethodException {

		BossRepositoryCustom customImplementation = new BossRepositoryImpl();
		RepositoryMetadata metadata = new DefaultRepositoryMetadata(BossRepository.class);
		RepositoryInformation information = new DefaultRepositoryInformation(metadata, RepositoryFactorySupport.class,
				customImplementation.getClass());

		Method customBaseRepositoryMethod = BossRepository.class.getMethod("save", Boss.class);
		assertThat(information.isCustomMethod(customBaseRepositoryMethod), is(true));

		BossRepository repository = mock(BossRepository.class);
		repository.save(new Boss());
	}

	interface BossRepository extends CrudRepository<Boss, Long>, BossRepositoryCustom {}

	interface BossRepositoryCustom {

		Boss save(Boss boss);
	}

	class BossRepositoryImpl implements BossRepositoryCustom {
		public Boss save(Boss boss) {
			return boss;
		}
	}

	static class Boss {}
}

[spring-projects-issues]

Gerard Nantel commented

Are you by any chance using Eclipse (and hence its built-in Java Compiler)?

I tried again with the exact code you provided and I get the same compilation error I was getting previously when using IntelliJ and when building via the command-line with Maven. I tried both Oracle HotSpot JDK and OpenJDK and get the same error with both.

I then tried with Eclipse Mars and it indeed works. However, as soon as I build with Maven, it fails.

I skimmed the Java spec to try to figure out why the call is ambiguous but I really need to take more time and step through it carefully to understand why: Chapter 15. Expressions

I also came across a similar issue where someone had something working in Eclipse but failed via Maven command-line and the conclusion was that the Eclipse Java compiler was not following the spec.

[spring-projects-issues]

Oliver Drotbohm commented

Quick heads up: you're right. The code I posted doesn't compile on the command line. The good news is: the fix I have in place that accommodates the changes in Mockito, will actually allow you to do the right and even better thing: just declare <S extends Boss> S save(S entity) … on your custom implementation class and the test will compile and succeed.

Fix coming in a bit. Would be awesome if you could give this a spin early morning. We're going to ship a release tomorrow. If that's too tight, nevermind. :)

[spring-projects-issues]

Gerard Nantel commented

Awesome! I would be delighted to try out the fix as soon as it is available.

[spring-projects-issues]

Oliver Drotbohm commented

This should be in place. Feel free to give the snapshots a spin!

[spring-projects-issues]

Gerard Nantel commented

I tried the latest 1.12.3 SNAPSHOT. It works great for my standard customization classes as long as I change the signatures from

MyEntity save(MyEntity entity);

to:

<S extends MyEntity> S save(S entity);

This is nicer anyways and I'm happy with that.

However, any of my more complex Customization classes that require auto-wiring (or perhaps a @Transactional annotation) do not work. It's because these classes are implemented with cglib proxies and this results in the save() methods on these classes having the signature:

MyEntity save(MyEntity entity);

without any generic type parameters. The parametersMatch() method then returns false for these as they are not generic method declarations and the custom method is never called.

If I change the newly added DefaultRepositoryInformation unit test for this case, which makes the use case very clear, by replacing SampleRepositoryImpl with:

	static class SampleRepositoryImpl {

		public Sample save(Sample entity) {
			return entity;
		}
	}

The test fails which is expected.

I then wrote a similar test as my original test which reproduces the issue, this time with JDK proxies (using an interface for the custom implementation so I could use JDK proxies and not cglib):

package org.springframework.data.repository.core.support;

import static org.hamcrest.Matchers.*;
import static org.junit.Assert.*;
import static org.mockito.Mockito.*;

import java.lang.reflect.InvocationHandler;
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;

import org.junit.Test;
import org.springframework.data.repository.CrudRepository;
import org.springframework.data.repository.core.RepositoryInformation;
import org.springframework.data.repository.core.RepositoryMetadata;

public class DefaultRepositoryInformation2UnitTests {

	@Test
	public void discoversCustomlyImplementedCrudMethodWithGenericParameters()
			throws SecurityException, NoSuchMethodException {

		BossRepositoryCustom customImplementation = (BossRepositoryCustom) Proxy.newProxyInstance(BossRepositoryCustom.class.getClassLoader(),
				new Class[] { BossRepositoryCustom.class },
				new InvocationHandler() {
			@Override
			public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
				return null;
			}
		});
		
		RepositoryMetadata metadata = new DefaultRepositoryMetadata(BossRepository.class);
		RepositoryInformation information = new DefaultRepositoryInformation(metadata, RepositoryFactorySupport.class,
				customImplementation.getClass());

		Method customBaseRepositoryMethod = BossRepository.class.getMethod("save", Object.class);
		assertThat(information.isCustomMethod(customBaseRepositoryMethod), is(true));

		BossRepository repository = mock(BossRepository.class);
		repository.save(new Boss());
	}

	interface BossRepository extends CrudRepository<Boss, Long> {}

	interface BossRepositoryCustom {
		public <S extends Boss> S save(S boss);
	}

	static class Boss {}
}

Let me know if my example is not clear. At first I thought I would remove auto-wiring so I rewrote my customization beans with required argument constructors but then realized I also had some @Transactional annotations in some places, which again required cglib. I could move to AspectJ weaving for @Transactional annotations but I assume these workarounds should not be necessary in general.

I will be available tomorrow in case you want me to test a fix.

[spring-projects-issues]

Gerard Nantel commented

Also, I noticed another detail (unrelated to the above issue). The following line will always be false, even for methods with an Iterable parameter:

boolean isNotIterable = !parameterType.equals(Iterable.class); // line 405

This is because we are comparing a ResolvableType instance with a Class instance. I checked and ResolvableType overrides equals but respects that fact:

if (!(other instanceof ResolvableType)) {
     return false;
}

So I think that line was meant to be:

boolean isNotIterable = !parameterType.getRawClass().equals(Iterable.class);

I can't really take credit for noticing this myself. IntelliJ warned me about it :)

[spring-projects-issues]

Gerard Nantel commented

Hi Oliver,

I noticed this issue is now closed. Should I open a new issue for the problem with cglib proxied repositories, described in my second-to-last comment above? Also, for the equals() comparison described in the last comment above above, is a pull request the better way to report it, even for such a small change?

Thanks

[spring-projects-issues]

Oliver Drotbohm commented

Yes please go ahead for further improvements. I wanted to get this to generally work quickly to at least get that fix into the release but I am happy to follow up in a new ticket