-
Notifications
You must be signed in to change notification settings - Fork 696
GH-3121 Throw PropertyReferenceException for whitespace-starting Prop... #3321
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
Conversation
...e-starting PropertyPath Signed-off-by: mipo256 <mikhailpolivakha@gmail.com>
79bb31e
to
153fa5a
Compare
I'll add the Integration test for mongodb to make sure that this case works
One issue is that MongoDB uses an abstraction that isn't fully applicable to how MongoDB represents paths. Accepting property paths that start with a whitespace is indeed odd, and we should look forward to reject these unless there's a good reason to keep them. Java properties cannot start with a whitespace, however, other JVM languages can create method names containing whitespace.
One issue is that MongoDB uses an abstraction that isn't fully applicable to how MongoDB represents paths
I agree. I think the separate issue can be created for this in Spring Data MongoDB to think about the design around it. Since PropertyPath
is indeed not exactly the great fit for expressing paths within a mongodb document, at least it seems.
Java properties cannot start with a whitespace, however, other JVM languages can create method names containing whitespace.
Well, yeah, that is true and it is a valid concern.
Just my 2 cents 😄 : If Spring Data decide to consider the whitespace starting paths in general, that means we need to differentiate between hypothetical foo
, foo
and \tfoo
as paths. And then it becomes unclear how to reference those fields using for instance derived queries like:
public MyEntity `findByFoo(SomeType foo)` // It is unclear how to express ` foo` vs `\tfoo` vs `foo`...
Apart from that, it seems that the Kotlin Language, the one that is most often used apart from Java with Spring applications, does not permit the identifier to start with whitespace, see specification:
identifier : (letter | '_') (letter | digit | '_')*
So, my humble opinion is that it is not worth it to consider the paths that start with a whitespace. But I might be wrong.
CC: @odrotbohm @mp911de
646e077
to
6ac5cf8
Compare
1f86a58
to
3a61461
Compare
b0bbaec
to
834368c
Compare
Closes #3121
So, actually, the situation that is described in the original ticket is quite flawed in many ways.
My basic assumption is that
PropertyPath
cannot hold whitespace holding properties. Now, the way it works withproperties. key
as the PropertyPath in spring data mongodb - it actually don't, we still were unable to parse thePropertyPath
, but we handed the exception - thePropertyReferenceException
.The mongodb can handles property references like
properties. key
, so the spring-data-mongodb just passed it literally asand the mongodb engine would figure it out.
Therefore, I think, that the only correct fix that can be applied here is to just throw a correct exception, which is
PropertyReferenceException
. Theproperties. key
andproperties. Key
should not be considered validPropertyPath
instances.CC: @christophstrobl @mp911de