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

Problems using EnumerationFields in queries #8800

Open
AndreaPiovanelli opened this issue Jul 30, 2024 · 5 comments
Open

Problems using EnumerationFields in queries #8800

AndreaPiovanelli opened this issue Jul 30, 2024 · 5 comments

Comments

@AndreaPiovanelli
Copy link
Contributor

Since the format values are now saved, the Equals clause in queries doesn't work anymore.

This is a side effect of #8789 , which saves the EnumerationField value in a different way then before, as shown by the following screenshot from Orchard_Projections_StringFieldIndexRecord table:
image
As you can see, the new version of the Value column contains now the separator character ";" at the start and at the end of the actual value, even when only one value is selected.
There are two topics to discuss about:

  1. Values inside the table are now ambiguous and should be coherent, perhaps with a migration step replacing old values with new ones. As a side note: saving a content item is enough to replace the "old" value format with the new one and using the "Value" property of the field works as expected.
  2. Queries filtering on the Value of an EnumerationField do not work anymore, because the value is now saved with ";" characters even if a single option is selected. For this reason, the Equals clause cannot be used and currently needs to be replaced by the Contains clause. Creating a migration step to change queries isn't a viable solution: any suggestions on how to make EnumerationFields compatible with the old query usage?

I add to the discussion @MatteoPiovanelli-Laser and @BenedekFarkas because they worked on the referenced pull request and @sebastienros who approved it.

@AndreaPiovanelli
Copy link
Contributor Author

AndreaPiovanelli commented Aug 1, 2024

: Separator + value.Trim(Separator) + Separator);

If we do not add separators to the Value property of the field, queries seems to work as expected (using both Equals and Contains operators) and I haven't noticed any side effects.

@BenedekFarkas
Copy link
Member

It seems the changes weren't completely backwards-compatible. :(

Updating the StringFieldIndexRecord.Value and/or StringFieldIndexRecord.LatestValue entries in a migration is fairly simple, but we can also change the filter to use an additional condition to match the value both with and without the separators at the beginning/end.

@AndreaPiovanelli
Copy link
Contributor Author

I used Upgrade module to create a scheduled task to update EnumerationField values, because content items needed to be re-published to ensure infosets to be updated too. This may be moved to a migration to ensure the automatic upgrade of values and avoid the manual operation of a back office user, but this may be a little tricky because of circular dependencies between Orchard.Projections and Orchard.Fields. This is also a very long task, potentially.

I am still working on a viable solution to ensure query compatibility with the newer value format, because creating a new condition isn't really an acceptable solution, since most back office users usually lack the access to the query configuration options and, ideally, the "Equals" condition should still be the right one for single selection EnumerationFields.

If interested, you can find current work in progress on the branch https://github.com/LaserSrl/Orchard/tree/fix/enumerationfieldvaluesmigration, which is based on Laser's fork of Orchard but should give you an idea of what I am working on.

@AndreaPiovanelli
Copy link
Contributor Author

@BenedekFarkas in the same branch, I found a working solution to the "query" problem. I added a specific IFilterProvider that is almost identical to current ContentFieldsFilter, filtering the fields inside the "Describe" method to only work on EnumerationFields. Consequentially, I modified the original ContentFieldsFilter to exclude from its Describe method the EnumerationFields (https://github.com/LaserSrl/Orchard/blob/acda3e32b37370423cb8cac17c5f29adfe46cc7d/src/Orchard.Web/Modules/Orchard.Projections/Providers/Filters/ContentFieldsFilter.cs#L39).

The ApplyFilter function for the EnumerationFieldsFilter modifies the Value parameter of the context object to properly add ';' separator characters when needed (see https://github.com/LaserSrl/Orchard/blob/acda3e32b37370423cb8cac17c5f29adfe46cc7d/src/Orchard.Web/Modules/Orchard.Projections/Providers/Filters/EnumerationFieldsFilter.cs#L77).
Since Category and Name parameters for the filter are the same as the original content fields filter, this solution works without a migration (old filters for EnumerationFields are now applied using the new provider).

The Upgrade procedure for the fields is needed to make everything work properly and has to be executed separately for each tenant. I excluded the upgrade of the values through a migration because the operation potentially involves thousands of content items for each tenant and may lock all tenants for a long time.

I released the branch on a test environment and will post a pr for the community as soon as possible.

@BenedekFarkas
Copy link
Member

Thanks for the update, I'll check get back to this towards the end of September (after Harvest).

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

No branches or pull requests

2 participants