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.

Sign up for GitHub

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

Jump to bottom

fix: optional filters deserialization #771

Merged
merged 7 commits into from
Feb 3, 2022
Merged

fix: optional filters deserialization #771

merged 7 commits into from
Feb 3, 2022

Conversation

aallam
Copy link
Member

@aallam aallam commented Jan 28, 2022 •
edited
Loading

Q A
Bug fix? yes
New feature? no
BC breaks? no
Related Issue n/a
Need Doc update no

Describe your change

Fixes optionalFilters deserialization for following special cases:

  • ["A:1", "B:2"] -> [["A:1"], ["B:2"]]
  • "A:1", "B:2" -> [["A:1"], ["B:2"]]

@aallam aallam requested a review from Ant-hem January 28, 2022 16:03
Ant-hem
Ant-hem reviewed Jan 28, 2022
View reviewed changes
Copy link
Member

@Ant-hem Ant-hem left a comment

Choose a reason for hiding this comment

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

thanks for the fix!

I am not sure about the behavior of single string filter.

👋🏻 @godelized do you remember how "color:green,color:yellow" should be translated? is it [["color:green"],["color:yellow"]]

https://github.com/algolia/algoliasearch-client-java-2/blob/20793bf208053e13692d43d4d9237eb480ffa9c1/algoliasearch-core/src/test/java/com/algolia/search/JacksonParserTest.java#L174-L176

algoliasearch-core/src/test/java/com/algolia/search/JacksonParserTest.java Outdated Show resolved Hide resolved
algoliasearch-core/src/test/java/com/algolia/search/JacksonParserTest.java Outdated Show resolved Hide resolved
@Ant-hem Ant-hem requested a review from godelized January 28, 2022 16:15
@aallam @Ant-hem
Apply suggestions from code review
9e0fb5b 
Co-authored-by: Antoine Hémery <antoine.hemery@algolia.com>
@godelized
Copy link
Member

Thanks!

I am not sure about the behavior of single string filter.

👋🏻 @godelized do you remember how "color:green,color:yellow" should be translated? is it [["color:green"],["color:yellow"]]

It looks like it is the legacy format:
https://github.com/algolia/AlgoliaSaaS/blob/330cbefd4e3d36eba3565760a2e9ca96708c3ca4/algolia/api/json_parsers/DecodeJSONStringArray.cpp#L153-L181.
If I understand correctly the first level are separate groups (i.e. conjunctions) and inside-parenthesis items are assigned to the same group (i.e.) disjunctions.

So:

  • color:green,color:yellow is equivalent to ["color:green", "color:yellow"] and [["color:green"], ["color:yellow"]].
  • (color:green, color:yellow) is equivalent to [["color:green", "color:yellow"]].

To be validated with a test 😄.

@godelized
Copy link
Member

I did a pass on the tests :)

https://github.com/algolia/algoliasearch-client-java-2/pull/771/files#diff-b4728c7df2af885fb4a7070b3ce19d5902bf55c3fe38ef704f519a3ed8657beeR174-R180

  • color:green,color:yellow
  • for the engine it is equivalent to a and: [["color:green"],["color:yellow"]]
  • the test should use assertANDEDListResult instead of assertOREDResult

https://github.com/algolia/algoliasearch-client-java-2/pull/771/files#diff-b4728c7df2af885fb4a7070b3ce19d5902bf55c3fe38ef704f519a3ed8657beeR182-R194

  • ["color:green","color:yellow"]
  • for the engine it is equivalent to a and: [["color:green"],["color:yellow"]]
  • the test uses assertOREDListResult but I didn't find the definition, but since it's a and, the test should use assertANDEDListResult

https://github.com/algolia/algoliasearch-client-java-2/pull/771/files#diff-b4728c7df2af885fb4a7070b3ce19d5902bf55c3fe38ef704f519a3ed8657beeR196-R204

Looks good :)

@aallam
Copy link
Member Author

aallam commented Feb 1, 2022 •
edited
Loading

  • color:green,color:yellow
  • for the engine it is equivalent to a and: [["color:green"],["color:yellow"]]
  • ["color:green","color:yellow"]
  • for the engine it is equivalent to a and: [["color:green"],["color:yellow"]]

color:green,color:yellow and ["color:green","color:yellow"] are then equivalent! I've pushed a small changes to match the above.
Thank you @godelized for your help 🙌

@aallam aallam requested a review from Ant-hem February 1, 2022 10:43
@aallam aallam force-pushed the fix/optional_filters branch from 2a00803 to 6015b3e Compare February 1, 2022 11:19
@aallam aallam force-pushed the fix/optional_filters branch from 4240bf2 to 54c8b60 Compare February 2, 2022 10:15
Ant-hem
Ant-hem approved these changes Feb 2, 2022
View reviewed changes
Copy link
Member

@Ant-hem Ant-hem left a comment

Choose a reason for hiding this comment

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

thanks for the patch @aallam, we now cover all the use-cases. :)

godelized
godelized approved these changes Feb 3, 2022
View reviewed changes
Copy link
Member

@godelized godelized left a comment

Choose a reason for hiding this comment

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

Let's go 🚀.
The only concern I have is that if a client sends a weird filter string like (((A:1),B:2),(C:3),D:4), the engine is going to accept it (but the behavior is not correct imo). However when fetching it with the Java client, it will convert it to something different.
But I guess it's an edge case of something that is already a bug so its fine 👍.

@aallam aallam merged commit 0e58180 into master Feb 3, 2022
@aallam aallam deleted the fix/optional_filters branch February 3, 2022 16:53
@sbellone sbellone mentioned this pull request Feb 3, 2022
Merged
@mehmetaligok mehmetaligok mentioned this pull request Feb 16, 2023
Merged
@mehmetaligok mehmetaligok mentioned this pull request Feb 23, 2023
Merged
mehmetaligok added a commit to algolia/algoliasearch-client-go that referenced this pull request Feb 24, 2023
@mehmetaligok
fix: handle composableFilterOption deserialization for legacy usage (#…
301a2c5 
…718)

| Q                 | A
| ----------------- | ----------
| Bug fix?          | yes
| New feature?      | no
| BC breaks?        | no     
| Need Doc update   | no


## Describe your change

One-string legacy filter deserialization behavior is fixed to handle parenthesis usage with multiple groups.
Added more tests to cover possible usages.

**PS:** This behavior is already fixed in other client libraries. This implementation is based on those PRs which are fixing the same issue.

algolia/algoliasearch-client-java#771
algolia/algoliasearch-client-csharp#803

## What problem is this fixing?

When legacy one-string filters are used deserialization was not working as expected. 
Normally, it should work as below;
```
"color:green,color:yellow" => [["color:green"],["color:yellow"]]

"(color:green,color:yellow),color:blue" => [["color:green","color:yellow"], ["color:blue"]]

"(color:green,color:yellow)" => [["color:green","color:yellow"]]
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ant-hem Ant-hem Ant-hem approved these changes

@godelized godelized godelized approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aallam @godelized @Ant-hem