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

[DELTASPIKE-1160] Enable other Entity types as return from native repository queries #52

Closed

Conversation

Danny02
Copy link

@Danny02 Danny02 commented Jun 2, 2016

At the moment native queries can only return the same entity type as of the repository.

A special flag exist on the @query annotation to disable mapping, this enables the possibility to return unmapped types as String, Date or Integer.

This change uses the return type of the method with the @query annotation.
If the type is java.util.List, it uses the generic type.

It should be checked if this behaviour is correct for all JPA providers and if some special logging is needed.

This change also removes the flag from @query, I don't know if you want to retain it for backward compatibility reasons and mark it as deprecated.

@Danny02 Danny02 changed the title Enable other Entity types as return from native repository queries [DELTASPIKE-1160] Enable other Entity types as return from native repository queries Jun 2, 2016
@@ -88,4 +83,12 @@ else if (query.isNative())
return context.applyRestrictions(result);
}

private Class<?> getMethodResultEntityClass(Method m){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HI Danny, good catch, thats what happens when I'm looking at this too late in the evening.

Can you please commit this all under the original ticket? and can you expand this method to check for collection types?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sry, but which original ticket do you mean? DELTASPIKE-1089?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either is fine, your commit message doesn't include a JIRA ticket.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@Danny02 Danny02 force-pushed the native_query_generic_return_fix branch from 5a0b34f to 494d5cd Compare June 2, 2016 13:34
@johnament
Copy link
Contributor

Looks good to me. If no one else gets to it, I'll merge this evening (EDT). Thanks for the contrib!

@johnament
Copy link
Contributor

johnament commented Jun 3, 2016

Hi Danny,

Just ran your changes against wildfly 10. Its failing for:

Tests in error: 
  should_query_names(org.apache.deltaspike.data.impl.handler.EntityRepositoryHandlerTest): org.hibernate.MappingException: Unknown entity: java.lang.String

I think this is the difference between the original patch and the new changes. I think the spec expects the class passed in to be an entity for a typed query. By not using a typed query, it uses fallback logic to plain JDBC column mapping.

Also, please rebase your changes from our master. Finally, please take a look at our contribution guide, specifically our code formatting. http://deltaspike.apache.org/documentation/source.html

Thanks!

@Danny02
Copy link
Author

Danny02 commented Jun 3, 2016

ah ok, I think one can check if the return type is a registered entity type with the entitymanager.
I will try this tomorrow.

@Danny02 Danny02 force-pushed the native_query_generic_return_fix branch 2 times, most recently from 9e2c921 to 393bb57 Compare June 3, 2016 14:19
@Danny02
Copy link
Author

Danny02 commented Jun 3, 2016

Hi,

I think everything should be fine now with the code (tested it on wildfly 10 & used eclipse formatter).

A quick question about constributing. Should I upload a patch file to Jira or can you merge the changes directly from GitHub to the ASF repository?

*/
@Deprecated
boolean returnsEntity() default true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please delete this parameter, just like in your original PR. This was only added this week to address this problem originally.

@Danny02 Danny02 force-pushed the native_query_generic_return_fix branch from 393bb57 to eb8d216 Compare June 3, 2016 16:23
@Danny02
Copy link
Author

Danny02 commented Jun 3, 2016

Done, didn't notice that this was such recent code

@johnament
Copy link
Contributor

Also, regarding your question, I'm fine with pulling the branch from GH into our repo.

@johnament
Copy link
Contributor

johnament commented Jun 3, 2016

Both TomEE 7 and Wildfly 10 fail on this test:

TomEE:

should_query_names(org.apache.deltaspike.data.impl.handler.EntityRepositoryHandlerTest): Column not found: 0

Wildfly:

should_query_names(org.apache.deltaspike.data.impl.handler.EntityRepositoryHandlerTest): org.hibernate.exception.SQLGrammarException: could not execute query

In addition, I had to fix a CS issue:

-    QueryHint[] hints() default {};
+    QueryHint[] hints() default { };

@Danny02 Danny02 force-pushed the native_query_generic_return_fix branch from eb8d216 to 3f974c1 Compare June 6, 2016 10:36
@Danny02
Copy link
Author

Danny02 commented Jun 6, 2016

Hi,

I removed some formatting changes I commited from code I didn't touch. I also finally removed the now unused Query paramter (forgot that).

On the point of failed tests. I'm sorry, but I can't reproduce them. I have run the following builds in './deltaspike/modules/data/impl':

  • mvn clean test -P wildfly-build-managed -Dwildfly.version=10.0.0.Final
  • mvn clean test -P tomee7-build-managed

and both are successfull

@johnament
Copy link
Contributor

I'm not sure what happened, but its passing locally now, and I didn't see anything wrong explaining why it would fail. I've merged in, but had to rebase so if you wouldn't mind closing this PR. Thanks for the contribution!

@Danny02 Danny02 closed this Jun 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants