Explicit binding behavior dont work in v13

[jarlef]

Is there an existing issue for this?

• I have searched the existing issues

Product

Hot Chocolate

Describe the bug

Changing DefaultBindingBehavior to BindingBehavior.Explicit does not effect the behavior and all fields are bound implicit. I would expect that at least Author and Book type would have no fields bound and that a ObjecTypeDescriptor would be created defining the fields.

Steps to reproduce

1. dotnet new graphql
2. append

ModifyOptions(opt =>
{
    opt.DefaultBindingBehavior = BindingBehavior.Explicit;
});

Relevant log output

No response

Additional Context?

No response

Version

13.0.0-rc.1

[tobias-tengler]

The binding behavior affects only Code-first types, such as ObjectType<T>.

[jarlef]

@tobias-tengler seriously? Returning objects without a ObjectType will just auto bind everything? Also when i added ObjectType<Book> and ObjectType<Author> i still had to add descriptor.BindFieldsExplicitly(); for it not returning other fields than those specified by descriptor.Field(...)

We have a lot of resolver returning domain models where not all properties are intended for the client. + our EF contains a lot entities not intended for the client. Missing one ObjectType<TModel> with descriptor.BindFieldsExplicitly() will just expose everything. We have used the Explicit mode to ensure we have full control over our schema where we opt in and not add a lot of configuration to opt out. If this the new approach were we cannot trust that HC is not truly in explicit mode but just autobind if it cannot find a configuration, then we have to consider adding own models for HC to return and use Automapper or something to copy from our internal domain classes to a simpler model.

For a reference. Downgrading the "graphql" template project from v13 to v12.6 (+ removing the QueryType attribute and registering the Query class using AddQuery) everything works as expected. Query, Book and Author have no fields.

[tobias-tengler]

i still had to add descriptor.BindFieldsExplicitly(); for it not returning other fields than those specified by descriptor.Field(...)

If you set the DefaultBindingBehavior to BindingBehavior.Explicit, this sounds like a bug.

Downgrading [...] from v13 to v12.6 [...] everything works as expected

I think the behavior before was a bug and the intention of this feature was to just apply to Code-first Types.

But just to be sure: @michaelstaib the BindingBehavior should just work for Code-first Types, right? And it no longer "working" for Annotation-based is an intended breaking change we did this version?

[jarlef]

There will be a lot of security concerns of auto binding objects without ObjectType<TModel> descriptor as well as long as we had set the Explicit as default binding behavior. E.g a new developer returns a new entity from our database but forgot to add the ObjectType<TModel> but at the same time annotated it with [UseProjection] and that got through our code review. All navigation properties that does not have an ObjectType descriptor will be added to the HC schema with all properties (aka table columns). Suddenly internal tables are exposed and a table like User with password hash + salt are exposed as a consequence. (we don't have one but it is a thing that can happen with auto binding)

[tobias-tengler]

That's true. But in a production application you should at least have a schema snapshot test to prevent exactly this from happening...

[michaelstaib]

How do you bind the ObjectType? What we changed is that clean ObjectType<Foo> would now autobind. But it should not affect any ObjectType that inherits or has an explicit configuration delegate.

[michaelstaib]

Can you provide a minimal repro that shows your issue?

[jarlef]

Of course. I will publish a repro repo 👍

[michaelstaib]

protected override ObjectTypeDefinition CreateDefinition(
    ITypeDiscoveryContext context)
{
    var descriptor = ObjectTypeDescriptor.New<T>(context.DescriptorContext);

    _configure!(descriptor);
    _configure = null;

    // if the object type is inferred from a runtime time we will bind fields implicitly
    // even if the schema building option are set to bind explicitly by default;
    // otherwise we would end up with types that have no fields.
    if (context.IsInferred)
    {
        descriptor.BindFieldsImplicitly();
    }

    return descriptor.CreateDefinition();
}

During type initialization we now bind implicitly if the type was inferred. It might be that there is a bug with that ... once I have your reopro I will test what the issue is.

[jarlef]

@michaelstaib I have finally gotten around to create a repro repo https://github.com/jarlef/HotChocolateExplicitBinding
I have added 2 servers with unit tests that shows the breaking behavior.

[jarlef]

I think its okay with ending up with a startup exceptions due to no fields defined (as it does for v12 and earlier) when running in explicit mode.

There could however possible been an improvement to configuration of how binding is configured across types. E.g BindingBehavior.Hybrid. E.g binding fields on pure classes is auto set to explicit meaning there must be created an ObjectTypeDescriptor to define fields, but classes annotated with [ExtendObjectType], [QueryType] etc are clearly classes defined for HC purpose and can be implicitly bound. Enums are also something that could have been autobound in that mode unless there is an EnumType override. The most important thing is not the leak out more fields on EF / Domain layer entities

[michaelstaib]

The train of thought we had was very different of how you use this. So, far we in discussions thought of the status quo kind of as a bug. But in your case you are forcing declaration with the errors. I will reflect on how we can reconcile both use-cases.

[michaelstaib]

So... we have discussed this quite a bit now and will rework this.

Types with attributes will always bind implicitly since its an explicit decision in this case to bind implicit. Its equivalent to 'BindFieldsImplicitly()' on a type that overrides the default behavior.

Types that however have no attribute will bind explicitly like before. While it could be desirable to have even more fine grained control over the type inference it becomes also all more complex.

We will ship it with rc.5 and it feels quite well balanced now.

[jarlef]

Sounds good Looking forward to testing it out. 👍
You guys are doing really excellent work on both cool new features and videos tutorials 🥇