export content items based on SQL query results (Lombiq Technologies: OCORE-98)

[lampersky]

fixes #11405

[lampersky]

[Piedone]

Apart from this and this LGTM, please re-request review when ready.

Please keep comments, except for code suggestions that you apply, open, don't resolve them, so I'll be able to go through them next time.

[lampersky]

all code suggestions applied, the only thing which is left here is https://github.com/OrchardCMS/OrchardCore/pull/12045#discussion_r928324172

[Piedone]

OK, please let me know when that's done too.

[lampersky]

OK, please let me know when that's done too.

maybe instead of waiting for Sebastien input I will refactor Query and will add additional property, which could be used here?

[lampersky]

I think ReturnContentItems could be moved from LuceneQuery to Query. Imo, it is rather a common thing to return ContentItems by various Query implementations.

@sebastienros what do you think?

[Piedone]

@Skrypt since this is affected by and affects https://github.com/OrchardCMS/OrchardCore/pull/11052, can you please advise on this and this?

[Skrypt]

To be honest I'm not sure if we want to be able to ReturnContentItems from every type of Query this is why I left it for each inherited Query type to decide.

[Skrypt]

If I'm comparing each of these Query they also have in common Template but have a different name for

ReturnContentItems == ReturnDocuments

So, if we change the type then you possibly break the Query definitions in the database. This means that you need to cast per Query to get the proper field of these classes.

Also, do we want to move Template to its base Query class because it is common to all of them? Maybe not. The Query class itself defines the minimum requirement for a Query. Maybe it should be an Interface instead then.

So the Query is defining what is needed to define its source and a Schema for GraphQL to map it.

[Skrypt]

After thinking. What I suggest is that we make a different PR that would assess these changes and migration. But discuss with @sebastienros about this first.

[jtkech]

@Skrypt @Piedone @lampersky About the Query abstraction concern.

Maybe metadata, marker interfaces, virtual methods.

For example with a new virtual method to not break existing json settings.

In the base class Query

    public virtual bool ResultsOfType<T>() => typeof(T) == typeof(object);

Then for example in LuceneQuery

    public override bool ResultsOfType<T>() => ReturnContentItems
        ? typeof(T) == typeof(ContentItem)
        : base.ResultsOfType<T>();

The usage being

        var query = await _queryManager.GetQueryAsync(id);
        if (query.ResultsOfType<ContentItem>())
        {

        }

[Piedone]

Ah, that sounds nice, yes!

[lampersky]

Ah, that sounds nice, yes!

@Piedone suggestion applied

[Piedone]

Let me know if you're ready for another review.

[lampersky]

Let me know if you're ready for another review.

I'm ready. I can't request review, because PR is already awaiting review from you. @Piedone

[sebastienros]

I don't think the changes on the interface are required. When the step is executed we can just check that it is a content item. And the description of the steps can mention that the query needs to return content items or it won't be used. Optionally it could also handle strings (ids) and do the lookup.