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

REGRESSION: default resolvers #234

Closed
rafales opened this issue Jun 24, 2019 · 13 comments
Closed

REGRESSION: default resolvers #234

rafales opened this issue Jun 24, 2019 · 13 comments

Comments

@rafales
Copy link

rafales commented Jun 24, 2019

This commit: 9a0f740

started setting resolver on fields. Providing a resolver for all fields breaks usage of default_resolver (meta param to ObjectType), which our project was using. This introduced a silent error, where our default resolver was not called.

@jnak
Copy link
Collaborator

jnak commented Jun 24, 2019

Hello, Thanks for reporting the issue. Can you share more about how you relied on the default_resolver? I believe graphene-sqlalchemy had a big dependency on attr_resolver so it's not clear to me how it could work with a different default_resolver.

@rafales
Copy link
Author

rafales commented Jun 24, 2019

Up until the latest version (2.2.1) graphene-sqlalchemy did not use any custom resolvers.

We have something built on top of graphene that uses default_resolver for some access-related logic. Basically what happens is something like this:

attrs_dict = {
    "Meta": {
        "model": self.db_model,
        "default_resolver": self.default_field_resolver,
        "only_fields": only_fields or (),
        "exclude_fields": exclude_fields or (),
    }
}

graphene_type = type(
    self.db_model.__name__, (SQLAlchemyObjectType,), attrs_dict
)

@rafales
Copy link
Author

rafales commented Jun 24, 2019

Everything is fine and our default_field_resolver gets called in versions <2.2.1, but in 2.2.1 default_resolver never gets called for SQLAlchemyObjectType descendants.

@jnak
Copy link
Collaborator

jnak commented Jun 24, 2019

Would you mind explaining what type of logic you are putting in default_field_resolver? We need understand the use case if we are going to consider supporting explicitly it via test and documentation.

@dtamez
Copy link

dtamez commented Jun 27, 2019

In our case we had a few places where we had a resolver on a type but did not explicitly have the field on the type. The type was defined as a SQLAlchemyObjectType and the field(s) in question were not in the excluded fields list. Our resolvers were defined on the type and required extra logic before returning and could not simply be returned from the database. One example was images that were being stored in AWS S3 with presigned urls that had an expiration date.
Before 2.2.1 the resolvers for those fields were being called and executed. With 2.2.1 the resolvers no longer got called. For now we have pinned the lib to 2.2.0 until this gets fixed or a decision is made that it should not be fixed.

@davidmoss
Copy link

I experienced this regression too, where my resolver_<field> stopped getting called in v2.2.1 for objects in a queryset.

@jnak
Copy link
Collaborator

jnak commented Jul 3, 2019

Can anyone share an actual repro of the bug? thank you

@ticosax
Copy link
Contributor

ticosax commented Jul 9, 2019

@jnak please check #235 to repro the bug, if I understand correctly

@davidmoss
Copy link

@ticosax looks good to me

@jnak
Copy link
Collaborator

jnak commented Aug 2, 2019

Apologies for the late reply. I was on vacation and did not have time to look into this until this week.

Thanks @ticosax for the repro. I figured out what's going on. It's indeed a serious regression :/
I have a fix that is mostly done, I just need to address a TODO. I'll finish it early next week.
Here is the branch in case you need the fix now.

Cheers

@jnak
Copy link
Collaborator

jnak commented Aug 6, 2019

@davidmoss @ticosax @dtamez Here is the fix: #241

@rafales Does my PR fix your issue? It seems that the repro posted by @ticosax is different from your original issue with default_resolver. I don't understand how you are using default_resolver without interfering with SQLAlchemyObjectType resolve its fields. Can you share some more details?

@jnak
Copy link
Collaborator

jnak commented Aug 14, 2019

The fix has been released on 2.2.2.

@dtamez Closing for now since you haven't provided yet more detail. Feel free to re-open when you have more details to share.

@jnak jnak closed this as completed Aug 14, 2019
@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related topics referencing this issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants