Fix Source ID Handler #394
Conversation
Signed-off-by: phmai <[email protected]>
Signed-off-by: phmai <[email protected]>
Signed-off-by: phmai <[email protected]>
Signed-off-by: phmai <[email protected]>
Signed-off-by: phmai <[email protected]>
Signed-off-by: phmai <[email protected]>
Signed-off-by: phmai <[email protected]>
Signed-off-by: phmai <[email protected]>
Signed-off-by: phmai <[email protected]>
Code Coverage
Files
|
Signed-off-by: phmai <[email protected]>
Code Coverage
Files
|
| // PropertyOperationUtil.transformPropertyInPropertyOperationTree( | ||
| // propertyOp, SourceIdHandler::mapIntoTagOperation); | ||
| final Optional<ITagQuery> tagQuery = transformPropertyOperation(propertyOp); | ||
| if (tagQuery.isPresent()) { |
There was a problem hiding this comment.
If you're using Optional you can use ifPresent with lambda so you won't need to call get afterwards :)
tagQuery.ifPresent(presentTagQuery -> {
// ...
});
There was a problem hiding this comment.
Thanks, I will change this place. There is 1 other place I will keep like this, as I need to access the index of the loop.
| // propertyOp, SourceIdHandler::mapIntoTagOperation); | ||
| final Optional<ITagQuery> tagQuery = transformPropertyOperation(propertyOp); | ||
| if (tagQuery.isPresent()) { | ||
| readRequest.getQuery().setTags(tagQuery.get()); |
There was a problem hiding this comment.
By doing so, we will lose (override) the original tagQuery that was in ReadFeatures.
Is that intended?
If not (or we simply don't know) then we could to the following:
- Check if there are any tag queries in the original request
2a) If there is a tag query, combine it viaANDwith tag query translated from property. We can utilizaTagAndfor that.
2b) If there is no tag query in the original request, go with the current approach (set just translated property)
There was a problem hiding this comment.
But first, let's try to establish if tag query and property query can be both present here.
My intuition is that it is totally possible but TBH I did not analyze the flow
There was a problem hiding this comment.
That's true, I missed this case. Yes there could be already existing tag query which should be kept. Will push the fix like you outlined.
| final Optional<ITagQuery> tagQuery = transformPropertyOperation(propertyOp); | ||
| if (tagQuery.isPresent()) { | ||
| readRequest.getQuery().setTags(tagQuery.get()); | ||
| if (isFullyConvertedToITagQuery(propertyOp)) { |
There was a problem hiding this comment.
I like that you extracted the method for complex boolean check 👍
| @@ -114,14 +125,66 @@ private Optional<String> getSourceIdFromFeature(NakshaProperties properties) { | |||
| } | |||
| } | |||
|
|
|||
| public static Optional<ITagQuery> mapIntoTagOperation(PQuery propertyOperation) { | |||
| private static boolean isFullyConvertedToITagQuery(IPropertyQuery propertyQuery) { | |||
There was a problem hiding this comment.
A bit misleading name here IMO - we don't do anything with iTagQuery - we just check if property query is"singular"
There was a problem hiding this comment.
The name makes sense only as the function is called in specific places after
final Optional<ITagQuery> tagComponent = transformPropertyOperation(propertyComponent);
if (tagComponent.isPresent())
It indicates that the property query (clause/fragment) has already been converted into tag query (clause/fragment) successfully (and in case of PAnd checking also the size to make sure nothing remains). If it's "fully converted", we can remove the property query (clause/fragment).
But I'm open to new name suggestions :b
| pAnd.removeAll(toRemove); | ||
| if (tagAnd.isEmpty()) { | ||
| return Optional.empty(); | ||
| } else if (tagAnd.size() == 1) { |
There was a problem hiding this comment.
I wonder if these simplifications are really needed - they will happen only if the client will specify property AND with single tiem, right?
There was a problem hiding this comment.
It's for cases where a given property query PAnd is not fully transformed to tag query. Let's take the simplest example, a read features request with only this property query, no tag or other query:
(p.foo=bar && p.@ns:com:here:mom:meta.sourceId=abc)
The second clause can be converted into if tag xyz_source_id_abc exists, but the first stays the same. As we resolve this PAnd, we assume that it can possibly be converted fully into tag query, so the result is also a TagAnd. But after conversion we have a PAnd with a single component (the 1st clause unchanged) and a TagAnd with also one single component (the 2nd clause transformed).
Technically these kinds of query are allowed, but I'm not sure if the other handler components and ultimately the storages can resolve those correctly, considering these can be nested as well, like a PAnd containing only 1 PAnd containing only 1 PAnd... So we have 2 separate simplifications there, one for PAnd and one for TagAnd.
Signed-off-by: phmai <[email protected]>
Code Coverage
Files
|
No description provided.