Implements #7921 extend query to support latest versions by HermesSbicego-Laser · Pull Request #7923 · OrchardCMS/Orchard

HermesSbicego-Laser · 2017-12-20T10:39:04Z

Implements #7921, branched from #7920 cause we need the LatestValue column to be there.
When a query is created/edited it is possible to specify the Content Version to query (Published or Latest).
Based on that choice the HqlQuery will filter

the right VersionOption
the right column of the (*)IndexFilterRecord

Based on that choice the HqlQuery will order

using the right column of the (*)IndexFilterRecord

- Adds Indexes to increase performances in queries - Adds Handler to store LatestValue during Update event - Adds Service to Get and Set the LatestValue of a field - Adds heavy projections index tables update to the Upgrade module

…ables locks during import

…e part's lifecycle - Manage Queries using ScopeVersion informations

…he query

sebastienros · 2017-12-21T20:31:21Z

+
+            return 5;
+        }
+        public int UpdateFrom5() {


Move to UpdateFrom4. And update the Create method to create the table correctly including all subsequent changes, then return 5.

Ok, I will threat this PR and the #7920 one as they were the same PR, so I can aggregate UpdateFrom4 and 5

sebastienros · 2017-12-21T20:33:38Z

+            SchemaBuilder.AlterTable("DecimalFieldIndexRecord", table => table
+            .AddColumn<decimal>("LatestValue"));
+
+            //Adds indexes for better performances in queries


Two comments:

We need to talk about the "necessity" of these indices

You are reusing the same index name multiple times, is it an issue? should it be unique per database, or no otherwise table prefix would break it?

Necessity of these indexes: do you mean SQL indexes or the "LatestValue" columns? For SQL indexes, I think at least IX_FieldIndexPartRecord_Id should be created because it is a foreign key "in fact". The index over PropertyName column is for projections and HqlQueries so I think they should be there. I don't see a good reason why they shouldn't be there

Same index name: I seem to remember that Indexes name should be unique per table and not per database. At least in SQL Server, so i don't see an issue here. Perhaps it's not the same in MySql or Postgre?

sebastienros · 2017-12-21T20:35:38Z


            // generate the predicate based on the editor which has been used
-            Action<IHqlExpressionFactory> predicate = fieldTypeEditor.GetFilterPredicate(context.State);
+            dynamic fullState = context.State;


Why dynamic ? You can't access the VersionScope property directly?

fieldTypeEditor.GetFilterPredicate (2 lines above) method use a dynamic parameter as input. I need that information within IFieldTypeEditor.GetFilterPredicate in order to return the right column (Value or LatestValue) to query.

sebastienros · 2017-12-21T20:36:22Z

-                ? context.Query.OrderBy(relationship, x => x.Asc("Value")) 
-                : context.Query.OrderBy(relationship, x => x.Desc("Value"));
+            context.Query = ascending
+                ? context.Query.OrderBy(relationship, x => x.Asc(context.VersionScope == QueryVersionScopeOptions.Latest ? "LatestValue" : "Value"))


Maybe create a method in the context to get the field name (based on the scope). I have seen this multiple times.

Ok, i will try

sebastienros · 2017-12-21T20:38:05Z

                    Query = groupQuery,
-                    State = FormParametersHelper.ToDynamic(sortCriterion.State)
+                    State = FormParametersHelper.ToDynamic(sortCriterion.State),
+                    VersionScope = queryRecord.VersionScope


Instead of duplicating this property in all context objects, can't we just pass the parent Query which already has the value?

Do you mean to add the QueryPart to the context?

sebastienros · 2017-12-21T20:39:44Z

+@using Orchard.Projections;
+
+<fieldset>
+    @Html.LabelFor(m => m.VersionScope, T("Query Version Scope"))


"Version Scope" is not very intuitive. "Versions" only seems better. What do we use in Lucene indices?

Ok for "Versions", or "Content versions"?

sebastienros · 2017-12-21T20:42:42Z

+        }
+
+        [HttpPost, ActionName("Index")]
+        public ActionResult IndexPOST() {


Shouldn't it do it by batch of content items? If there are too many of them this will fail. There are some examples doing it in other upgrade controllers.

Can you address me to an example where I can copy from? thanks.

Found it by myself in InfosetController.cs

sebastienros · 2017-12-21T20:47:48Z

        }
+        private void Updated(UpdateContentContext context, FieldIndexPart fieldIndexPart) {
+            if (context.UpdatingItemVersionRecord.Latest) { // updates projection draft indexes only if it is the latest version
+                foreach (var part in fieldIndexPart.ContentItem.Parts) {


Try to externalize this method, and use a lambda as the differences.

OK, I will try...

sebastienros · 2017-12-21T20:49:07Z

+using Orchard.Projections.Models;
+
+namespace Orchard.Projections.Services {
+    public class DraftFieldIndexService : IDraftFieldIndexService {


I understand this is to prevent a breaking change. But I'd prefer to see it merged into the existing service in the dev branch. Can you file an issue for this? I will assign it for 1.11.

Ok, i will do it.

opened issue #7925

…lterContext and SortCriteriaContext: used in ProjectionManager - Adds a method which returns the columnName to Sort/Filter to FilterContext and SortCriteriaContext: used in ContentFieldsSortCriteria - Externalize the logic to calculate the column to index - Merge Update 4 and 5 - Modified the label "Query Version Scope" into "Content's version"

BenedekFarkas · 2018-09-12T14:03:49Z

Does the code expect that the Query's version scope can only be Published or All? I'm asking because #8111.

HermesSbicego-Laser · 2018-09-13T06:35:36Z

In this PR, the Query's version scope can be Published or Latest.

HermesSbicego-Laser · 2018-09-13T06:56:45Z

@BenedekFarkas I read a bit the code in your #8112 PR. Adding the filter on the right versionOption of the content is not sufficient. Should be changed other code. In particular where we test the VersionOption in order to choose the right column (Value/LatestValue) to check/order.
see:

BenedekFarkas · 2018-09-13T09:36:33Z

@HermesSbicego-Laser good point, I only saw the addition of LatestValue after submitting my PR - I'll look into it and update accordingly, thanks!

HermesSbicego-Laser added 6 commits December 18, 2017 23:40

- Adds a new Transaction foreach content update in order to prevent t…

791940a

…ables locks during import

- Adds Index over FieldIndexPartRecord_Id

3b7bb8e

- Adds Column ScopeVersion to QueryPartRecord and manage it during th…

9387576

…e part's lifecycle - Manage Queries using ScopeVersion informations

- Manage the scope of the query within Fields

d494562

- Manage the sort criterion for fields based on the VersionScope of t…

758d91f

…he query

sebastienros reviewed Dec 21, 2017

View reviewed changes

HermesSbicego-Laser added 2 commits December 26, 2017 13:10

- Batch upgrade for LatestValue

1824551

HermesSbicego-Laser mentioned this pull request Dec 27, 2017

Merge DraftFieldIndexService.cs into FieldIndexService.cs #7925

Closed

HermesSbicego-Laser added 2 commits December 27, 2017 16:09

- async ajax call in order to prevent freezing UI

b1b063d

- Created a context for lambda diversification

335d617

HermesSbicego-Laser mentioned this pull request Jan 4, 2018

Fix #7903 adding the appropriate columns to index fields latest values #7920

Closed

sebastienros merged commit 5800e18 into OrchardCMS:1.10.x Jan 4, 2018

HermesSbicego-Laser deleted the issue/7921_Extend_Query_To_Support_LatestVersions branch January 5, 2018 07:34

HermesSbicego-Laser mentioned this pull request Jan 28, 2018

Fields are not indexed if content is not published #7903

Closed

protoncm mentioned this pull request Aug 7, 2018

Error when running Orchard.Projection Test #8093

Merged

This was referenced Aug 25, 2019

#6960 Queries returning Draft Content Items and with more tokens available #6961

Closed

Queries returning Draft Content Items and with more tokens availables for filters #6960

Closed

Uh oh!

Conversation

HermesSbicego-Laser commented Dec 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenedekFarkas commented Sep 12, 2018

Uh oh!

HermesSbicego-Laser commented Sep 13, 2018

Uh oh!

HermesSbicego-Laser commented Sep 13, 2018

Uh oh!

BenedekFarkas commented Sep 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants