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

Fix field usage on prepared input types with original value #4902

Merged

Conversation

rmosolgo
Copy link
Owner

@rmosolgo rmosolgo commented Apr 5, 2024

Fixes #4864

Along the way, I noticed that InputObject#prepare was called twice when that method returned itself. By removing the first call, this solution worked out nicely.

@@ -215,8 +215,7 @@ def coerce_input(value, ctx)
if resolved_arguments.is_a?(GraphQL::Error)
raise resolved_arguments
else
input_obj_instance = self.new(resolved_arguments, ruby_kwargs: resolved_arguments.keyword_arguments, context: ctx, defaults_used: nil)
input_obj_instance.prepare
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @rmosolgo, we tried upgrading to the latest version of graphql ruby and we are seeing an error where input types aren't having their prepare method called. For example, one of our input types looks like this:

class CreditNoteLineItemInput < Types::BaseInputObject

    argument :amount, Types::MoneyInput, required: true, description: "The amount to credit."
    argument :charge_id,
             ID,
             required: true,
             loads: Types::Charge,
             as: :charge,
             description: "The charge being credited."

    def prepare
      { payable: charge, amount: amount }
    end
  end

When our unit tests run, input arguments for this type aren't being provided as a hash. I haven't had time to prepare a test case, but wondering if you may have introduced a bug on this PR?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may have introduced a bug on this PR?

Always possible! Perhaps related: #4932

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this bug might be related to both loads: and def prepare being used, but it's not that simple. I added a spec in c72f700 and it passed.

I'd like to track this down! Could you please open an issue with some details:

  • The source for the input object (shared above)
  • An example query which replicates the bug
  • The field where the input object is used
  • Anything else you think might be relevant

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When our unit tests run

How do your unit tests look like? Is this a unit test for the input object?

At GitLab, we had to update our unit tests because we were testing by calling coerce_isolated_input. The #prepare now happens outside of this method so we had to call #prepare manually. See https://gitlab.com/gitlab-org/gitlab/-/commit/4dc3e1866fc89387f1a7de004124751e0bda59fb

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am taking another look and I think this problem may be related to some related libraries that we use at Shopify. I pulled the latest versions of all the Graphql gems we use and I am no longer seeing this error. Will report back and open an issue if I have anything definitive. Apologies for the false alarm.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, please open a new issue if you find anything that we should investigate further 👍

@rmosolgo rmosolgo deleted the fix-field-usage-on-prepared-input-types-with-original-value branch April 26, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants