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

OGM-1426 InfinispanRemote native queries follow up #1022

Closed
wants to merge 14 commits into from

Conversation

Projects
None yet
2 participants
@DavideD
Copy link
Member

commented May 17, 2018

Follows #1012

https://hibernate.atlassian.net/browse/OGM-1426
https://hibernate.atlassian.net/browse/OGM-1463

This is a refactoring of the initial PR sent by @fax4ever:

  • Reduce the amount of regular expressions needed for the parser
  • Add new test cases to check queries with strange cases and use of spaces
  • Give additional information when the query is invalid
  • Test that expected exceptions are thrown

Using regular expressions for the parser scares me a bit, but it seems to work for now because we don't need to extract lot of info from the native query. We should consider switching to a proper parser in the future like Parboiled.

Please @fax4ever, have a look at this and if you find something that doesn't make sense let me know.
Otherwise, let's merge it and close this issue.

Thanks a lot

@DavideD DavideD self-assigned this May 17, 2018

@DavideD DavideD requested a review from fax4ever May 17, 2018

@fax4ever

This comment has been minimized.

Copy link
Contributor

commented May 17, 2018

Thank you @DavideD. I'm going to review extra commits.

@fax4ever
Copy link
Contributor

left a comment

Thank you @DavideD, reducing the amount of regular expressions needed for the parser was very good. I have just a minor doubt about the 5642f87.

@@ -34,7 +34,7 @@

@Override
public QueryParsingResult parseQuery(SessionFactoryImplementor sessionFactory, String queryString, Map<String, Object> namedParameters) {
throw new UnsupportedOperationException( "The Infinispan Remote query parser supports parameterized queries" );
throw new UnsupportedOperationException( "The Infinispan Remote query parser doesn't support parameterized queries" );

This comment has been minimized.

Copy link
@fax4ever

fax4ever May 17, 2018

Contributor

I think that supports was right. Like in Neo4jBasedQueryParserService.
In a nutshell: the client driver supports parameters natively, like in Neo4j, unlike MongoDB. Thus, from what I understood, other method must be called, because the passer supports parameterized queries.

This comment has been minimized.

Copy link
@DavideD

DavideD May 17, 2018

Author Member

Thank you @DavideD, reducing the amount of regular expressions needed for the parser was very good. I have just a minor doubt about the 5642f87.

Thanks @fax4ever,. Got it. Could you remove that commit and merge everything else, please? The message seems misleading but that's an issue for another time.

This comment has been minimized.

Copy link
@fax4ever

fax4ever May 17, 2018

Contributor

Sure!

@fax4ever

This comment has been minimized.

Copy link
Contributor

commented May 17, 2018

Rebased and merged (all except 5642f87)! Thanks ;)

@fax4ever fax4ever closed this May 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.