Add generic check for function return type

[deepesh-verma]

Fixes: #215
Signed-off-by: deepesh-verma deepeshvrm1@gmail.com

[pivotal-issuemaster]

@deepesh-verma Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-issuemaster]

@deepesh-verma Thank you for signing the Contributor License Agreement!

[garyrussell]

While this fixes the reported issue, unfortunately, I think it needs to be more sophisticated; we need to examine the generic parameter(s) Map<String, Foo> should match Map<String, Bar> if Bar is a subclass of Foo.

[deepesh-verma]

Created a method isParameterizedTypeAssignable to verify the actualTypeArguments with isAssignableFrom check. Please review and let me know if this is what we expect.

[deepesh-verma]

@garyrussell I have made the changes as requested. Can you please review?

[garyrussell]

Thanks for the contribution; sorry for the delay.

After further review; this is incorrect:

	protected static class GenericInheritanceReturnTypeRecover {

		@Retryable
		public Map<String, Double> bar(String name) {
			return Collections.singletonMap("bar", 0.0);
		}

		@Recover
		public Map<String, Number> quux(RuntimeException re, String name) {
			return Collections.singletonMap("bar", 3);
		}

	}

(My original comment was wrong). We can't return a wider type from the recover method; we can, however, return a narrower type, if there is no direct match. This should work...

	protected static class GenericInheritanceReturnTypeRecover {

		@Retryable
		public Map<String, Number> bar(String name) {
			return Collections.singletonMap("bar", 0.0);
		}

		@Recover
		public Map<String, Double> quux(RuntimeException re, String name) {
			return Collections.singletonMap("bar", 3);
		}

	}

...but I think it would be ok to only allow direct matches because the above would add a lot more complexity.

However, we can't stop at the first level of generics; we need to examine nested generic types; this should work, but does not.

	protected static class MoreComplexGenericInheritanceReturnTypeRecover {

		@Retryable
		public Map<String, List<Integer>> foo(String name) {
			return Collections.singletonMap("foo", Collections.singletonList(0));
		}

		@Retryable
		public Map<String, List<Double>> bar(String name) {
			return Collections.singletonMap("bar", Collections.singletonList(0.0));
		}

		@Recover
		public Map<String, List<Integer>> baz(RuntimeException re, String name) {
			return Collections.singletonMap("foo", Collections.singletonList(1));
		}

		@Recover
		public Map<String, List<Double>> qux(RuntimeException re, String name) {
			return Collections.singletonMap("bar", Collections.singletonList(1.0));
		}

	}

	@Test
	public void genericMapListIntegerReturnTypeRecoverMethod() {
		RecoverAnnotationRecoveryHandler<?> fooHandler = new RecoverAnnotationRecoveryHandler<Integer>(
				new MoreComplexGenericInheritanceReturnTypeRecover(),
				ReflectionUtils.findMethod(
						MoreComplexGenericInheritanceReturnTypeRecover.class, "foo",
						String.class));
		@SuppressWarnings("unchecked")
		Map<String, List<Integer>> recoverResponseMapRe = (Map<String, List<Integer>>) fooHandler
				.recover(new Object[] { "Aldo" }, new RuntimeException("Planned"));
		assertEquals(1, recoverResponseMapRe.get("foo").get(0).intValue());
	}

	@Test
	public void genericMapListDoubleReturnTypeRecoverMethod() {
		RecoverAnnotationRecoveryHandler<?> barHandler = new RecoverAnnotationRecoveryHandler<Double>(
				new MoreComplexGenericInheritanceReturnTypeRecover(),
				ReflectionUtils.findMethod(
						MoreComplexGenericInheritanceReturnTypeRecover.class, "bar",
						String.class));
		@SuppressWarnings("unchecked")
		Map<String, List<Double>> recoverResponseMapRe = (Map<String, List<Double>>) barHandler
				.recover(new Object[] { "Aldo" }, new RuntimeException("Planned"));
		assertEquals(1.0, recoverResponseMapRe.get("bar").get(0).doubleValue(), 0.0);
	}

The second test matches baz instead of qux.

[deepesh-verma]

Thanks for the feedback and the test cases; It helps a lot to clearly understand what is expected here;
Let me try to work on below points -

• Generic return type match should only be a direct match (not narrower)
• We should consider all the nested level of generics for matching method return type

[deepesh-verma]

Changes -

• Update isParameterizedTypeAssignable to recusrively call itself, if the actualTypeArguments are again instanceof ParameterizedType.
• Add test cases with generic return type having 3 levels of nesting to test this out.

@garyrussell, Please review.

[garyrussell]

It's best to add a new commit rather than force-pushing so we can more easily see what changed.

[deepesh-verma]

Sorry for that. I will make sure to add new commits going forward.
Please let me know if there is anything I can do to make it easier for now.

[garyrussell]

Sorry for the delay; finally merged as 4d2bb2b and ae3ad4a