OrchardCMS / Orchard

Orchard is a free, open source, community-focused Content Management System built on the ASP.NET MVC platform.
https://orchardproject.net
BSD 3-Clause "New" or "Revised" License
2.37k stars 1.12k forks source link

Problems using EnumerationFields in queries #8800

Open AndreaPiovanelli opened 1 month ago

AndreaPiovanelli commented 1 month ago

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 commented 1 month ago

https://github.com/OrchardCMS/Orchard/blob/6e4fefd873dc17a9b098493afbe511a614cce82e/src/Orchard.Web/Modules/Orchard.Fields/Fields/EnumerationField.cs#L14 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 commented 1 month ago

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 commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 1 week ago

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