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

Error when disabling Lucine or elastic search feature #16372

Open
giannik opened this issue Jun 26, 2024 · 7 comments · May be fixed by #16402
Open

Error when disabling Lucine or elastic search feature #16372

giannik opened this issue Jun 26, 2024 · 7 comments · May be fixed by #16402
Labels
Milestone

Comments

@giannik
Copy link
Contributor

giannik commented Jun 26, 2024

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

@MikeAlhayek MikeAlhayek added this to the 2.0 milestone Jun 27, 2024
Copy link

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.

MikeAlhayek added a commit that referenced this issue Jun 27, 2024
@gvkries
Copy link
Contributor

gvkries commented Jun 28, 2024

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
Copy link
Member

@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
Copy link
Contributor

gvkries commented Jun 28, 2024

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

@gvkries
Copy link
Contributor

gvkries commented Jun 28, 2024

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

@MikeAlhayek
Copy link
Member

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
Copy link
Contributor

gvkries commented Jul 1, 2024

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.

MikeAlhayek added a commit that referenced this issue Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment