Prevent crashing crashing the GraphQL schema when SEO feature is also enabled

[tropcicstefan]

Fixes #16140

[MikeAlhayek]

@tropcicstefan Let's not add this dependency. Two alternative ways to do this

First approach, move the SeoMetaQueryObjectType class into the Media module and register it using the following startup "in the Media startup

[RequireFeatures("OrchardCore.Apis.GraphQL", "OrchardCore.Seo")]
public class Startup : StartupBase
{
    public override void ConfigureServices(IServiceCollection services)
    {
        services.AddObjectGraphType<MetaEntry, MetaEntryQueryObjectType>();
        services.AddObjectGraphType<SeoMetaPart, SeoMetaQueryObjectType>();
    }
}

Second approach, Move both MediaFieldQueryObjectType and MediaAssetObjectType out of OrchardCore.Media and place them into OrchardCore.Media.Core. New the the SEO module can depend on OrchardCore.Media.Core and you are good to go.

However, I am thinking to combine the 2 solutions. The first solution is better but at the same time we should also move MediaFieldQueryObjectType and MediaAssetObjectType out of OrchardCore.Media and place them into OrchardCore.Media.Core.

[tropcicstefan]

ok, but want to know what is the reasoning, why shouldn't it depend on media when it's already dependant with manifest?

[MikeAlhayek]

@tropcicstefan In OC, we do not allow a module to reference another module. It's okay for a module to reference on core projects but not a module. It is okay to have dependencies using the Mainifest file, but not project reference.

[MikeAlhayek]

Can you please review the newly added references to double check if all these are really needed?

[MikeAlhayek]

Let's not change the namespace to avoid users having to fix their code if they are using this class

[MikeAlhayek]

Abstraction module should never depend on a core module

[MikeAlhayek]

Let's not change the namespace here.

[gvkries]

This does not actually fix the underlaying problem. The problem is that GraphQL types are registered as scoped (as introduced by #15319). Therefore resolving them while building the schema as a singleton fails.

We will encounter these errors again, if we don't fix the way types are registered for GraphQL.

[gvkries]

I think #16143 is a simpler solution and also fixes similar bugs we may encounter in the future.

[tropcicstefan]

I think #16143 is a simpler solution and also fixes similar bugs we may encounter in the future.

probably true, but this should also be fixed because it only works for now because of ObjectGraphTypeFieldProvider.GetObjectGraphType. Fields should be mapped to correct types

[gvkries]

probably true, but this should also be fixed because it only works for now because of ObjectGraphTypeFieldProvider.GetObjectGraphType. Fields should be mapped to correct types

Hmm, I don't think that has to change. Using the base class (ObjectGraphType<MediaField> in this case) is enough and it avoids having to reference the actual field type. No need to move any type around.

[tropcicstefan]

it's ridiculous that I need as much instances of ObjectGraphType for a mediafield as there are nodes of mediafield in schema graph its a base type there should be only one instance

[gvkries]

it's ridiculous that I need as much instances of ObjectGraphType for a mediafield as there are nodes of mediafield in schema graph its a base type there should be only one instance

Sorry, I don't understand. Can you elaborate?

[MikeAlhayek]

@tropcicstefan if you are using the OC direct code "main branch", can you see if the crash issue you are having is fixed? Is you are using the preview, please wait until tomorrow and see if the latest previews fixes you issues.

[tropcicstefan]

it's ridiculous that I need as much instances of ObjectGraphType for a mediafield as there are nodes of mediafield in schema graph its a base type there should be only one instance

Sorry, I don't understand. Can you elaborate?

If you have five content types and each has one mediafield, with your code there will be five instances of MediaFieldQueryObjectType

[gvkries]

it's ridiculous that I need as much instances of ObjectGraphType for a mediafield as there are nodes of mediafield in schema graph its a base type there should be only one instance

Sorry, I don't understand. Can you elaborate?

If you have five content types and each has one mediafield, with your code there will be five instances of MediaFieldQueryObjectType

Oh, I see. Yes that is true, but there is no easy way around.

Edit: I tested that quickly and it doesn't look like that. It' instantiating the type only once.

[MikeAlhayek]

@tropcicstefan did you get a chance to try out the latest code and see if your issue is fixed without this PR?

[tropcicstefan]

Bug was on latest preview that day, and other pr fixes it. I don't think it's correct to convert to transient, but ok

[MikeAlhayek]

I don't think it's correct to convert to transient, but ok

@tropcicstefan but did you confirm that it actually fixes your issue? The schema is built once and using transient isn't bad since it's not created on every request.

[tropcicstefan]

yes it fixes the issue, confirmed

[gvkries]

Bug was on latest preview that day, and other pr fixes it. I don't think it's correct to convert to transient, but ok

Originally, all GraphQL types were registered as singletons, but that had other problems after upgrading the GraphQL version itself (e.g. #15304, because the schema has to be rebuilt when dynamic types have changed).
'Scoped' can't be used here either, because the schema itself is still a singleton. So all that remains is the transient... 🤷‍♂️