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

Resolve to None when empty ID was passed #3497

Merged
merged 3 commits into from Dec 20, 2018

Conversation

maarcingebala
Copy link
Member

@maarcingebala maarcingebala commented Dec 20, 2018

This is a small improvement to the get_node_or_error function - if empty ID was passed as an argument it should immediately resolve to None. Without this change, multiple errors will be returned if empty String was passed as an ID to a mutation:

Currently, if we pass an empty string as an ID argument in a mutation, it will result in two errors:

{
    "field": "category",
    "message": "Couldn't resolve to a node: "
},
{
    "field": "category",
     "message": "This field cannot be null."
}

This PR changes this behavior to immediately return None in the get_node_or_error function so that the actual object resolver is not fired. This will result in only one meaningful error:

{
    "field": "category",
     "message": "This field cannot be null."
}

Edit: not related, but this PR also includes one minor TS types update, as it was out-of-date.

Pull Request Checklist

  1. Privileged views and APIs are guarded by proper permission checks.
  2. All visible strings are translated with proper context.
  3. All data-formatting is locale-aware (dates, numbers, and so on).
  4. Database queries are optimized and the number of queries is constant.
  5. Database migration files are up to date.
  6. The changes are tested.
  7. The code is documented (docstrings, project documentation).
  8. GraphQL schema and type definitions are up to date.
  9. Changes are mentioned in the changelog.

@maarcingebala maarcingebala added the graphql Issues related to the GraphQL API label Dec 20, 2018
@codecov
Copy link

codecov bot commented Dec 20, 2018

Codecov Report

Merging #3497 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3497      +/-   ##
==========================================
+ Coverage   89.89%   89.89%   +<.01%     
==========================================
  Files         241      241              
  Lines       13107    13109       +2     
  Branches     1322     1323       +1     
==========================================
+ Hits        11782    11784       +2     
  Misses        921      921              
  Partials      404      404
Impacted Files Coverage Δ
saleor/graphql/core/mutations.py 92.75% <100%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d615eb5...1a4c428. Read the comment docs.

@maarcingebala maarcingebala merged commit 989d5fb into saleor:master Dec 20, 2018
@maarcingebala maarcingebala deleted the improve-get-node branch December 20, 2018 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
graphql Issues related to the GraphQL API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants