Validate Meta.fields and Meta.exclude on DjangoObjectType

[berekuk]

Resolves #840.

[jkimbo]

Nice, thanks @berekuk !

[erik-megarad]

This broke my application. I have a @property on a model that I want to be included as a field in the DjangoObjectType node for that model. Previously this worked just fine with a resolver.

IMO this change should be reverted as it's an unexpected breaking change with no apparent workaround. I think a better term solution would be to also check if there's a resolve_ method for that field, so at least you could specify a resolver to get around this check.

[berekuk]

Uh, sorry.

I'm not interested in this patch anymore, since I decided to go with Ariadne instead for my project, so I don't mind it being reverted or whatever.

[jkimbo]

@subwindow apologies for breaking your application. We try hard to make sure that minor releases don't contain breaking changes.

However in this case I don't think this is a breaking change since it's exposing a misconfiguration with your setup. Adding a value to fields that isn't a Django model field would have just been silently ignored before this change which is unexpected. If you define a field and resolver on your DjangoObjectType they will be included in the type regardless of whether the name is added to the fields list. All you have to do @subwindow is only include the model fields in the fields list and everything should work fine.

[erik-megarad]

OK, @jkimbo I understand. It's just not super clear because the word "field" is ambiguous. I had to read the source to figure it out.

[jkimbo]

Yep I appreciate it's not super clear. It should be thought of as "model fields" really. It's also somewhere were Graphene-Django differs from Django Rest Framework which is not ideal.

[NathHorrigan]

I also agree this is a bad merge and has broken my library that is dependent on graphene-django. I don't always all my fields to be visible and this change still throws errors even when I exclude them in my class meta.

[jkimbo]

@NathHorrigan can you give an example of what you mean by "change still throws errors even when I exclude them in my class meta"? Errors should only be thrown if you're defining a field that doesn't exist on the django model.

[NathHorrigan]

It's a bit harder to provide an example as we dynamically build graphene-django types but I'm getting errors when my graphene types ignore a field on my model (by setting exclude_fields).

Is this the intended effect?

[NathHorrigan]

Error: Exception: "tagged_items" exists on model <class 'images.models.CustomImage'> but it's not a field.

[erik-megarad]

IMO what would be nice is to rework the error message to specifically say "Django model field" and to change it to a deprecation warning and not an error.

[jkimbo]

@subwindow @NathHorrigan I've been thinking about this more and I think you're right this should be a warning rather than an exception for now. Also it shouldn't warn on custom fields either. I'll create a PR to that effect.

[jkimbo]

PR: #862

[jkimbo]

@subwindow @NathHorrigan I've just released v2.8.1 that downgrades the errors if a field doesn't exist on a model to warnings. It also takes custom fields into account. Sorry for the inconvenience.

[erik-megarad]

Thank you!

[ulgens]

@jkimbo v2.8.1 came with other errors 🙂 It gives warning for all the model fields in exclude saying they are not model fields and should be removed from fields, which are not even in fields. I had to downgrade to 2.8.0 to silence these misleading warnings.

[jkimbo]

Ah I didn't check the exclude list... Sorry @ulgens, I'll fix this right away.

[jkimbo]

Created a PR to fix it @ulgens : #873

Will try and release it as v2.8.2

[ulgens]

Thank you so much @jkimbo I'm not able to test it at the moment but i can ping you back when i can test.