Allow deserialization of the base 'Query' type #16388

gvkries · 2024-07-01T10:38:47Z

If a feature with an IQuerySource that has been saved to a QueriesDocument gets deactivated, deserializing the document will fail because the base type Query cannot be deserialized.

This PR fixes this by adding a parameterless constructor to Query itself. It also ensures that any previously saved data from the queries document is preserved by using an JSON extension property.

Because I found no way to preserve the original type information when the base Query type gets re-serialized, I added an feature event handler, that fixes missing type information in the QueriesDocument when a previously used feature is reactivated.

Fixes #16372

/cc @MikeAlhayek

…erived type gets deactivated.

MikeAlhayek · 2024-07-01T15:58:59Z

@gvkries what will happen when a feature is enabled, disable, and then enabled again?

A user will create queries (SqlQuery), then disabled the Sql feature, updates the document, then enable Sql feature. Will then lose the SqlQuery data previously provided?

MikeAlhayek · 2024-07-01T16:16:04Z

I think adding the default constructor alone my be enough without the next for the handler. Can you please try that out?

gvkries · 2024-07-01T16:17:29Z

No, the query should be kept intact, because the feature event handler will fix it. Basically the sequence would be:

Feature is enabled and QueriesDocument contains a SqlQuery with some settings.
You disable the feature and edit/create a different query afterwards (e.g. Lucene).
During that editing, the SqlQuery will be deserialized as the base type Query, the original data is kept in the extension property. But when the document is saved again, the additional type information in the $type discriminator gets removed. It is now serialized as Query only.
If the feature gets enabled again, the event handler will find a query of the base type with the source name Sql in the document and replace it with the derived type information.

Maybe there is a more elegant way to do this.

MikeAlhayek · 2024-07-01T18:22:06Z

@gvkries Did you try to only add a default constructor without the event handler?

I think if the serializer is able to serialize, everything will work as expected without having to worry about fixing the document

gvkries · 2024-07-01T18:49:37Z

@gvkries Did you try to only add a default constructor without the event handler?

I think if the serializer is able to serialize, everything will work as expected without having to worry about fixing the document

No, that is not enough. If you re-serialize a Query instance while its derived type is not available, you loose the original type information.

E.g. a original Lucene query may look lke:

"RecentBlogPosts": {
    "$type": "OrchardCore.Search.Lucene.LuceneQuery, OrchardCore.Search.Lucene",
    "Index": "Search",
    "Template": "{\r\n  \u0022query\u0022: {\r\n    \u0022term\u0022: { \u0022Content.ContentItem.ContentType\u0022: \u0022BlogPost\u0022 }\r\n  },\r\n  \u0022sort\u0022: {\r\n    \u0022Content.ContentItem.CreatedUtc\u0022: {\r\n      \u0022order\u0022: \u0022desc\u0022,\r\n      \u0022type\u0022: \u0022double\u0022\r\n    }\r\n  },\r\n  \u0022size\u0022: 3\r\n}\r\n",
    "ReturnContentItems": true,
    "Name": "RecentBlogPosts",
    "Source": "Lucene",
    "Schema": "{\r\n    \u0022type\u0022: \u0022ContentItem/BlogPost\u0022\r\n}"
}

After Lucene gets deactivated and the query is deserialized and serialized as a Query you will get this:

"RecentBlogPosts": {
    "Name": "RecentBlogPosts",
    "Source": "Lucene",
    "Schema": "{\r\n    \u0022type\u0022: \u0022ContentItem/BlogPost\u0022\r\n}",
    "Index": "Search",
    "Template": "{\r\n  \u0022query\u0022: {\r\n    \u0022term\u0022: { \u0022Content.ContentItem.ContentType\u0022: \u0022BlogPost\u0022 }\r\n  },\r\n  \u0022sort\u0022: {\r\n    \u0022Content.ContentItem.CreatedUtc\u0022: {\r\n      \u0022order\u0022: \u0022desc\u0022,\r\n      \u0022type\u0022: \u0022double\u0022\r\n    }\r\n  },\r\n  \u0022size\u0022: 3\r\n}\r\n",
    "ReturnContentItems": true
}

MikeAlhayek · 2024-07-01T19:25:29Z

No, that is not enough. If you re-serialize a Query instance while its derived type is not available, you loose the original type information.

I think that would be enough. The data will still be in the database but won't be mapped to a concrete type. In addition to that, I would also not sure these queries on the UI to prevent someone from modifying that record until that feature is enabled again.

MikeAlhayek · 2024-07-01T22:57:47Z

@gvkries please check out the PR #16392 which solves the same issue but without the need for the handler.

gvkries · 2024-07-02T07:22:04Z

@gvkries please check out the PR #16392 which solves the same issue but without the need for the handler.

That is a good addition, but the handler is still required because even if the feature gets re-enabled, the QueriesDocument contains only the base Query and will not deserialize the derived type anymore. So the user would loose all queries, if we don't 'repair' the document.

gvkries · 2024-07-02T07:36:53Z

Note the missing $type discriminator in my sample above.

MikeAlhayek

@sebastienros can you think of a better idea here to eliminate the QueriesDocumentUpdater class?

MikeAlhayek · 2024-07-02T15:50:25Z

src/OrchardCore.Modules/OrchardCore.Queries/Services/QueriesDocumentUpdater.cs

+    {
+        var document = await _documentManager.GetOrCreateMutableAsync();
+
+        if (!ValidateDocument(document) && FixupDocument(document))


I think !ValidateDocument(document) is unnecessary and should be removed. I think using FixupDocument will be enough since we'll only iterate once instead of possible two. Also maybe call this method RepairDocument instead.

MikeAlhayek · 2024-07-02T15:50:58Z

src/OrchardCore.Modules/OrchardCore.Queries/Services/QueriesDocumentUpdater.cs

+        // Check for any query that has no specific type information and try to fix it up with a derived type.
+        // The type information is lost if a user edits the document while it contains a query for a feature that
+        // has been disabled.


decorate this method with this comment instead of the body.

MikeAlhayek · 2024-07-02T15:53:10Z

src/OrchardCore.Modules/OrchardCore.Queries/Services/QueriesDocumentUpdater.cs

+            if (query.GetType() == typeof(Query))
+            {
+                var sample = _querySources.FirstOrDefault(x => x.Name == query.Source)?.Create();
+
+                if (sample != null)
+                {
+                    var json = JObject.FromObject(query);
+
+                    document.Queries[kv.Key] = (Query)json.ToObject(sample.GetType());
+                    hasChanged = true;
+                }
+            }


Save indentation

Suggested change

if (query.GetType() == typeof(Query))

{

var sample = _querySources.FirstOrDefault(x => x.Name == query.Source)?.Create();

if (sample != null)

{

var json = JObject.FromObject(query);

document.Queries[kv.Key] = (Query)json.ToObject(sample.GetType());

hasChanged = true;

}

}

if (query.GetType() != typeof(Query))

{

continue;

}

var sample = _querySources.FirstOrDefault(x => x.Name == query.Source)?.Create();

if (sample != null)

{

var json = JObject.FromObject(query);

document.Queries[kv.Key] = (Query)json.ToObject(sample.GetType());

hasChanged = true;

}

sebastienros · 2024-07-02T19:30:11Z

Suggestion is to split the QueriesDocument into separate documents (one for each query) using a migration.

A mitigation to the problem could be to delete the queries when the feature is disabled.

gvkries and others added 2 commits July 1, 2024 12:27

Allows deserializing the base 'Query' type in case a feature with a d…

f549ddc

…erived type gets deactivated.

Fix formatting

9d90044

Merge branch 'main' into gvkries/queries-16372

ad6aa07

MikeAlhayek reviewed Jul 2, 2024

View reviewed changes

gvkries closed this Jul 2, 2024

gvkries deleted the gvkries/queries-16372 branch July 8, 2024 11:46

This was referenced Aug 1, 2024

Monthly community metrics report for 2024-07-01..2024-07-31 #16509

Closed

Monthly community metrics report for 2024-07-01..2024-07-31 SGuidone/OrchardCore#46

Open

Monthly community metrics report for 2024-07-01..2024-07-31 NinaRocket/OrchardCore#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow deserialization of the base 'Query' type #16388

Allow deserialization of the base 'Query' type #16388

gvkries commented Jul 1, 2024

MikeAlhayek commented Jul 1, 2024

MikeAlhayek commented Jul 1, 2024

gvkries commented Jul 1, 2024

MikeAlhayek commented Jul 1, 2024 •

edited

Loading

gvkries commented Jul 1, 2024

MikeAlhayek commented Jul 1, 2024 •

edited

Loading

MikeAlhayek commented Jul 1, 2024

gvkries commented Jul 2, 2024 •

edited

Loading

gvkries commented Jul 2, 2024

MikeAlhayek left a comment

MikeAlhayek Jul 2, 2024 •

edited

Loading

MikeAlhayek Jul 2, 2024

MikeAlhayek Jul 2, 2024

sebastienros commented Jul 2, 2024

Allow deserialization of the base 'Query' type #16388

Allow deserialization of the base 'Query' type #16388

Conversation

gvkries commented Jul 1, 2024

MikeAlhayek commented Jul 1, 2024

MikeAlhayek commented Jul 1, 2024

gvkries commented Jul 1, 2024

MikeAlhayek commented Jul 1, 2024 • edited Loading

gvkries commented Jul 1, 2024

MikeAlhayek commented Jul 1, 2024 • edited Loading

MikeAlhayek commented Jul 1, 2024

gvkries commented Jul 2, 2024 • edited Loading

gvkries commented Jul 2, 2024

MikeAlhayek left a comment

Choose a reason for hiding this comment

MikeAlhayek Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

MikeAlhayek Jul 2, 2024

Choose a reason for hiding this comment

MikeAlhayek Jul 2, 2024

Choose a reason for hiding this comment

sebastienros commented Jul 2, 2024

MikeAlhayek commented Jul 1, 2024 •

edited

Loading

MikeAlhayek commented Jul 1, 2024 •

edited

Loading

gvkries commented Jul 2, 2024 •

edited

Loading

MikeAlhayek Jul 2, 2024 •

edited

Loading