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

Fix #91: remove escaping/decorating of dotted path Shredder produces #92

Merged
merged 5 commits into from
Feb 10, 2023

Conversation

tatu-at-datastax
Copy link
Contributor

@tatu-at-datastax tatu-at-datastax commented Feb 9, 2023

This PR removes escaping as initially added since it is no longer needed and complicates matching of filters.

It also adds basic integration test cases to validate that dotted notation filter matching now works.

@tatu-at-datastax tatu-at-datastax requested a review from a team as a code owner February 9, 2023 21:47
@tatu-at-datastax tatu-at-datastax self-assigned this Feb 9, 2023
Copy link
Contributor

@ivansenic ivansenic left a comment

Choose a reason for hiding this comment

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

I am not sure if this works in all cases. I generally don't understand why are we not using the standard json path notation, where arrays are represented as:

path.array[0]

and there is clear distinction between

path.array.0

* This means that property name like {@code "a.b"} is encoded as {@code "a\\.b"}.
* <p>No escaping is used so path itself is ambiguous (segment "1" can mean either Array index #1 or
* Object property "1") without context document, but will NOT be ambiguous when applied to specific
* target document where context node's type (Array or Object) is known.
Copy link
Contributor

Choose a reason for hiding this comment

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

What when the node type is not known, for example if I do a $set on some.path.1 and path is not existing in the some object. Will the path be array or object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JsonPath is only used for query index / matching, not for Update operations. Update operations will require more advanced handling, but they have context document (document to update) for that decision.
I will not be using JsonPath there -- although there's also possibility that since JsonPath is so simplistic we may just go back to String for this use case unless we find need for something beyond String.

Copy link
Contributor

@maheshrajamani maheshrajamani left a comment

Choose a reason for hiding this comment

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

LGTM

@tatu-at-datastax
Copy link
Contributor Author

I am not sure if this works in all cases. I generally don't understand why are we not using the standard json path notation, where arrays are represented as:

path.array[0]

and there is clear distinction between

path.array.0

This is because path we build here needs to match Mongo/Mongoose "dot notation" which is simplistic: the only use case is to build index fields (query_text_values etc).

I agree that for any other use case (esp. if we had to reconstruct document) we would use something that guarantees uniqueness.

@tatu-at-datastax tatu-at-datastax merged commit c4ff8a5 into main Feb 10, 2023
@tatu-at-datastax tatu-at-datastax deleted the tatu/91-remove-path-escaping-from-shredder branch February 10, 2023 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants