HHH-18106 - @CheckHQL is reporting error when named query contains valid Java constant

[cigaly]

See Jira issue HHH-18106

When named query in entity method annotated with @CheckHQL contains valid Java constant, processor should not report error

[hibernate-github-bot]

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

[gavinking]

Currently we're testing the query validation just as an integrated part of the code generation, with tests in /tooling/metamodel-generator/src/test/java. This isn't perfect by a long shot, but it's good enough until we sit down and come up with something much better. And, yes, It's true that it doesn't exercise @CheckHQL, but that annotation is essentially trivial.

I'm not at all a fan of the approach you're proposing here. This test is verbose and is going to be difficult to understand and maintain. I would prefer if you just follow the existing (admittedly imperfect) approach.

[cigaly]

I absolutely agree that this test is more than ugly. I will be more than happy if I will be able to test it in the same way as the other tests. But the problem is that existing CompilationTest's are being executed with inherited class path. This results in all used classes to be present in class loader in BasicDotIdentifierConsumer.resolvePath. This way problem will never be visible in tests, but it will be still present in standalone example. That is the only reason why I've created such an ugly test case.

[gavinking]

Not sure what you mean.

[cigaly]

Sorry if my explanation is (little bit) messy :-(

The point is that, when processor if running standalone, class loader does know anything about classes that is processing. When BasicDotIdentifierConsumer.resolvePath is trying to resolve class using ClassLoaderService, ClassNotFoundException is thrown, caught and ignored, then next statement is throwing SemanticExpression with message "Could not interpret expression ...". Which is correct outcome when non existing class is referenced.

If testing is done with existing approach (test class that extends CompilationTest) ClassLoaderService already knows all classes under src/test/java, BasicDotIdentifierConsumer.resolvePath can find referenced class, and everything works like there is no problem present.

Below is simple test class that will pass. Normally, first two assertions should pass, but third and fourth not.

public class ConstantInNamedQueryTestv2 extends CompilationTest {

	@Test
	@WithClasses({ CookBook.class })
	public void testCase() {
		assertPresenceOfFieldInMetamodelFor( CookBookWithCheck.class, "QUERY_FIND_BAD_BOOKS" );
		assertPresenceOfFieldInMetamodelFor( CookBookWithCheck.class, "QUERY_FIND_GOOD_BOOKS" );

		assertPresenceOfMethodInMetamodelFor( CookBookWithCheck.class, "findBadBooks", EntityManager.class );
		assertPresenceOfMethodInMetamodelFor( CookBookWithCheck.class, "findGoodBooks", EntityManager.class );
	}
}

[gavinking]

If testing is done with existing approach (test class that extends CompilationTest) ClassLoaderService already knows all classes under src/test/java

That's weird and surely wrong. We must have something set up wrong.

[cigaly]

OK, then I will check to see if there is some problem with current approach to testing. My assumption was that this is working as expected, so I've created all this "mess" to work differently.

[gavinking]

Awesome, thanks!

[cigaly]

Every source file under tooling/metamodel-generator/src/main/java will be compiled and .class visible to class loader. Those that must not be visible should be placed elsewhere. Most logical place is tooling/metamodel-generator/src/main/resources.

Possible solutions to isolate some of sources used for testing are either to add new parameter to WithClasses annotation or to create new annotation (like existing WithMappingFiles or WithProcessorOption). I've implemented first option, but this can be easily changed.

The thing that I do not like in this draft solution is that I had to duplicate several methods (and only those needed for testing in this particular case) in org.hibernate.processor.test.util.TestUtil to accept class name instead of class itself.

Of two test methods in test class, one is accepting only one source which is placed in resources directory, and other where entity class is in sources directory, but class containing constants is in resources.

[gavinking]

LGTM, thanks so much for this!

[gavinking]

Suggested change

	return e.getKind() == ElementKind.FIELD &&
	e.getModifiers().contains( Modifier.STATIC ) &&
	e.getModifiers().contains( Modifier.FINAL );
	return e.getKind() == ElementKind.FIELD
	&& e.getModifiers().contains( Modifier.STATIC )
	&& e.getModifiers().contains( Modifier.FINAL );

[gavinking]

Are we done with this one? Shall we merge it?

[gavinking]

Are we done with this one? Shall we merge it?

@cigaly? Status?

[cigaly]

Yes, you can merge that.

[gavinking]

Sorry, @cigaly I now notice there are conflicts, can you resolve them please and then I will merge?

[cigaly]

OK

[cigaly]

@gavinking I've resolved conflict and pushed commit.

[gavinking]

Excellent, thanks @cigaly!