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

Support Optional on repository query parameters [DATACMNS-768] #1228

Closed
spring-projects-issues opened this issue Sep 24, 2015 · 5 comments
Closed

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented Sep 24, 2015

Christopher Smith opened DATACMNS-768 and commented

Spring Data repositories can now return me an Optional.empty() if I don't have a matching result, but I still have to manually check before querying whether I actually have a key to search on. (In my case, I have an optional promo code that might not be present.) I can define an Optional<String> as a repository query parameter, but if I pass an empty to it, I still get an NPE.

I would like to be able to do something like this:

Optional<Promotion> findByCodeIgnoreCase(Optional<String> code);
...

Optional<Promotion> promo = promos.findByCodeIgnoreCase(Optional.ofNullable(promoCode));

(In the meantime, perhaps the repositories shouldn't allow Optional as a parameter type at all, since they don't really work correctly.)


Affects: 1.11 GA (Gosling)

Issue Links:

  • DATAMONGO-2486 Add support for optional query parameters in repository methods
    ("is duplicated by")
  • DATAJPA-809 ParameterBinder should use ParameterAccessor
@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 7, 2015

Oliver Drotbohm commented

That's a great idea and should be easily doable on top of our return value handling already

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 7, 2015

Oliver Drotbohm commented

This is in place in the snapshots. I needed to tweak Spring Data JPA slightly to make sure it uses the unwrapping, too (see DATAJPA-808). I've also tweaked the Spring Data JPA example here

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 7, 2015

Christopher Smith commented

Are you sure that you linked the right issue on JPA? Additionally, as far as I can tell, this doesn't actually resolve the issue. DATAMONGO, at least, does permit sending java.util.Optional as of Gosling, but Optional.empty() produces an NPE.

Furthermore, I realized that the semantics here aren't clearly defined and need to be. Specifically, passing empty to an optional parameter could mean:

  1. ignore the parameter in the query
  2. query for parameter == null
  3. query for "parameter's field is missing" (in MongoDB, { "$exists": false}, which is distinct from present-but-null)
  4. if empty is provided for any field, the repository returns an empty result set

The first option, I think, is best dealt with by a QueryPredicate, especially given the support in MVC for collecting request parameters into one. Distinguishing between the other options is tricky, and I'm not sure that any single one of them is Clearly Right, although #2 is probably wrong. (The "stream position" suggests that #4 makes sense, but the "imperative logic position" would lean toward #3.)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 8, 2015

Oliver Drotbohm commented

Yes, I am. Even with the fix here, JPA didn't work as it was still using the argument array directly. Using the ParameterAccessor fixed with this ticket, the JPA parameter binding now sees the unwrapped values instead of the raw parameter. Regarding MongoDB, I guess it might just have not failed during execution but probably didn't return any reasonable results. Mongo's more lenient when it comes to invalid queries as it's basically query by example.

Regarding the optional parameter semantics: we currently simply treat Optional.empty() as if you had passed in null in the non-optional scenario. That's how you actually get to answers to all of your scenarios:

  1. Does not apply, as we also don't just drop parameter bindings if you provide null. For dynamic query creation, we support Querydsl, JPA specifications, etc.
  2. That's what we basically do. In the JPA case we apply extra null handling to accommodate the specialties of SQL.
  3. There's a dedicated keyword for that plus requires a boolean value.
  4. That's quite contrived I guess but of course can be implemented in a manual implementation.

It feels like you're looking into this from an HTTP request binding level, but that actually is an orthogonal topic and probably worth discussing for Spring Data REST. In the Querydsl support we introduced recently we explicitly support customizing the binding via a dedicated callback interface.

Summing this up, what we added here is basically a treatment of Optional.empty() (as well as the Guava version) like null is treated on the non-Optional case. I'd argue that's what people usually introduce the use of Optional for in the first place to prevent null inside the codebase.

Does that make sense?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 8, 2015

Christopher Smith commented

That's a perfectly sensible set of semantics, but it doesn't appear to be clearly documented, and other semantics could also make sense.

That said, I'm still not sure that this change fixes the problem as I originally tried to report it--passing Optional.empty() resulted in an NPE from the MongoDB repository proxy. Should this be testable on snapshot builds? (In our actual code, we just used Elvis to get around the query in the first place, even though it's a bit less functional-y.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants