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

FEATURE: Case-insensitive property value criteria #4725

Merged

Conversation

bwaidelich
Copy link
Member

@bwaidelich bwaidelich commented Nov 10, 2023

Adds support for the special case insensitive operators:

  • =~ equals
  • $=~ ends with
  • ^=~ starts with
  • *=~ contains

And makes sure, that the other string based operators (=, $=, ^= and *=) are case sensitive on all supported platforms.

Resolves: #4721
Related: #2600

Streamlines the behavior of the `ContentSubgraph` for string based
PropertyValueCriteria to be case sensitive (to be in sync with Neos < 8)

Related: #4721
@kitsunet
Copy link
Member

Note this is a bit complex topic, I know eel / fizzle separates between case(in)sensitive but if the implementation is correct in all cases is another question, also NodeDataRepository::findByProperties is case insensitive so far, so I think it would be fine (and probably more correct) to be insensitive here as well? Probably something to discuss.

Adds support for the special case _insensitive_ operators:
* `=~` equals
* `$=~` ends with
* `^=~` starts with
* `*=~` contains

Related: #2600
@bwaidelich bwaidelich changed the title BUGFIX: Case sensitive property value criteria FEATURE: Case-insensitive property value criteria Nov 10, 2023
@kitsunet
Copy link
Member

kitsunet commented Nov 10, 2023

Got to block this for a second here, I think this uncovers a different problem we should fix first and apply the solution to this as well.
The strange collation you found is probably a result of some defaults of your system, MY projections use utf8_unicode_ci (from the docs: "An alias for utf8mb3. In MySQL 8.0, this alias is deprecated; use utf8mb4 instead. utf8 is expected in a future release to become an alias for utf8mb4." )

I started to investigate a bit and the events table does use "proper" utf8mb4_unicode_ci and I think it's this here:
https://github.com/neos/eventstore-doctrineadapter/blob/main/src/DoctrineEventStore.php#L165

which seems missing from the schema construction we do for the contentrepository projections and I think we should definitely add it otherwise the charset and collaction are probably left to some system default which would be a bad idea.

See eg. here:
https://github.com/neos/contentgraph-doctrinedbaladapter/blob/9.0/src/DoctrineDbalContentGraphSchemaBuilder.php

Flow does set:

        defaultTableOptions:
          charset: 'utf8mb4'

So that seems to fit. IMHO we should first change all projections to actually adhere to this and then also use this to decide which collation to use in queries like these here.

Edit to make this clearer, my local system also has the projection tables on utf8mb3_unicode_ci the ones above were from my localbeach instances, so it seems indeed to depend on some default or other. Not good.
Tried to create a table with a varchar field in both local and localbeach and both came out different but neither of them with mb3 charset. This is so tricky, but I guess using the same configured default as we do for the eventstore would make a lot of sense.

@mhsdesign
Copy link
Member

mhsdesign commented Jan 31, 2024

How can we continue here?

The above has been fixed.

@bwaidelich
Copy link
Member Author

With db64f78 I hard-coded the case-sensitive search to collate properties and search term to utf8mb4_bin. I thought about making this a function or global constant of DoctrineDbalContentGraphSchemaBuilder but I don't think that this will be needed somewhere else (and if it turns out to be, we can still extract that).
I also removed the explicit collations for case insensitive search because this is already guaranteed with #4729

And since it is all covered with tests, this can me merged IMO

Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

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

Thanks looks good by reading ;)

Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

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

Thanks a lot for "bear"ing with me (SCNR), this looks great.

@mhsdesign mhsdesign merged commit 38a3b69 into 9.0 Feb 12, 2024
10 checks passed
@mhsdesign mhsdesign deleted the feature/4721-case-sensitivity-for-propertyvaluecriteria branch February 12, 2024 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Streamline case sensitive behavior of PropertyValueCriteria
3 participants