OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.23k stars 2.34k forks source link

Error when disabling Lucine or elastic search feature #16372

Open giannik opened 1 week ago

giannik commented 1 week ago

On the latest main branch ,while testing and evaluating elastic search and storing some queries I decided to disable Elasticsearch. But given that the queries documents contained existing elastic queries the following error occurs: The same happens if you disable Lucene feature and you have existing Lucene queries in the queries document.

image

github-actions[bot] commented 1 week ago

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

gvkries commented 1 week ago

Can't we just make Query deserializable by adding a parameterless constructor with a default name?

In that case we just need to take some extra care if the user edits the queries document, e.g. when saving a SQL query. Additional data and type information will be lost in that case, but we can work around that by keeping additional data in the Query object with the JsonExtensionDataAttribute:

[JsonExtensionData]
public Dictionary<string, JsonElement> ExtensionData { get; set; }

Because the type discriminator of the derived types are lost in that scenario as well, we also need to correct the document in the QueryManager when loading it, e.g. something like this:

private void FixupDocument(QueriesDocument document)
{
    // 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 when a user edits the document while a feature has been deactivated.
    if (document.Queries.Any(kv => kv.Value.GetType() == typeof(Query)))
    {
        foreach (var kv in document.Queries.ToArray())
        {
            var query = kv.Value;
            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());
                }
            }
        }
    }
}

I don't know if this is a feasible approach.

MikeAlhayek commented 1 week ago

@gvkries that is not a bad approach. But probably fix the document using a migration step instead of the document load.

Would you like to submit a PR for this?

gvkries commented 1 week ago

A migration is of cause better. I try to find some time for it next week. Have a nice weekend 😊

gvkries commented 1 week ago

Hmm, is it possible to run a migration every time a feature gets activated?

MikeAlhayek commented 1 week ago

No. But once you fix the old data once, you should be good. Any new data will be stored in the new format so we don't need to migrate it again.

gvkries commented 4 days ago

No. But once you fix the old data once, you should be good. Any new data will be stored in the new format so we don't need to migrate it again.

A migration is than not enough, because the QueriesDocument must be validated whenever a feature with an IQuerySource gets activated. I used an FeatureEventHandler instead.