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 transforming and validating nested inputs and arrays #462

Merged
merged 3 commits into from
Nov 21, 2019

Conversation

MichalLytek
Copy link
Owner

Closes #133 🔒

@MichalLytek MichalLytek added Bug 🐛 Something isn't working Solved ✔️ The issue has been solved labels Nov 10, 2019
@MichalLytek MichalLytek added this to the 1.0.0 release milestone Nov 10, 2019
@MichalLytek MichalLytek self-assigned this Nov 10, 2019
@MichalLytek
Copy link
Owner Author

@j Please review the changes - they are based on your #452 PR 😉

@codecov
Copy link

codecov bot commented Nov 10, 2019

Codecov Report

Merging #462 into master will increase coverage by 0.28%.
The diff coverage is 98.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #462      +/-   ##
==========================================
+ Coverage   94.74%   95.03%   +0.28%     
==========================================
  Files          74       75       +1     
  Lines        1238     1310      +72     
  Branches      234      247      +13     
==========================================
+ Hits         1173     1245      +72     
  Misses         62       62              
  Partials        3        3
Impacted Files Coverage Δ
src/schema/schema-generator.ts 96.69% <ø> (ø) ⬆️
src/resolvers/convert-args.ts 100% <100%> (ø)
src/resolvers/helpers.ts 100% <100%> (ø) ⬆️
src/resolvers/validate-arg.ts 100% <100%> (ø) ⬆️
src/helpers/types.ts 98.21% <50%> (+0.06%) ⬆️

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 20c81f4...430918d. Read the comment docs.

@j
Copy link
Contributor

j commented Nov 11, 2019

@MichalLytek looks like familiar code! :P haha but looks great. I'll get to playing with it soon. I moved forward with a lot of internal development and was going to add validation at some point later. Thanks for this.

@MichalLytek
Copy link
Owner Author

@j Soooo have you checked the changes? Does it work with edge use cases? 😄

@j
Copy link
Contributor

j commented Nov 18, 2019

@MichalLytek woops, I wrote that on my phone and thought it was merged. I'll test this now.

@j
Copy link
Contributor

j commented Nov 18, 2019

@MichalLytek based on our current code-base where we use inheritance on the root level of an input and nested inputs, I was able to confirm that validation works and all inputs are mapped correctly. I toyed with doing deep nested with inheritance and those were converted properly as well.

Thanks for this!

@j
Copy link
Contributor

j commented Nov 18, 2019

@MichalLytek one thing I just saw, and I'm not sure if this is a bug or not. If you have a nullable input, it's creating the object with undefined fields.

@j
Copy link
Contributor

j commented Nov 18, 2019

Extra info:

The below mutation has a nullable input, in this case, I left it out and errors happen.

mutation createCart {
  createCart {
     ...
  }
}
[
            "TypeError: Cannot read property 'email' of undefined",
            "    at tree.getFields.reduce (/Users/graphql/node_modules/type-graphql/dist/resolvers/convert-args.js:54:27)",
            "    at Array.reduce (<anonymous>)",
            "    at convertToInput (/Users/graphql/node_modules/type-graphql/dist/resolvers/convert-args.js:52:42)",
            "    at convertValueToInstance (/Users/graphql/node_modules/type-graphql/dist/resolvers/convert-args.js:71:11)",
            "    at convertValuesToInstances (/Users/graphql/node_modules/type-graphql/dist/resolvers/convert-args.js:78:12)",
            "    at Object.convertArgToInstance (/Users/graphql/node_modules/type-graphql/dist/resolvers/convert-args.js:104:12)",
            "    at Object.<anonymous> (/Users/graphql/node_modules/type-graphql/dist/resolvers/helpers.js:17:76)",
            "    at Generator.next (<anonymous>)",
            "    at /Users/graphql/node_modules/tslib/tslib.js:110:75",
            "    at new Promise (<anonymous>)"
          ]

@MichalLytek
Copy link
Owner Author

MichalLytek commented Nov 19, 2019

Thanks for the report, @j! That's the use cases I was thinking of 😄

I've checked the nullable fields but they are handled by convertToType.
But the if the value of nullable arg @Arg("input", { nullable: true }) input?: SampleNestedInput is null, then it failed, so I've fixed it 💪

If you have a nullable input, it's creating the object with undefined fields.

It's still the same case like with the createCart and Cannot read property 'email' of undefined? Or it's a different one cause I don't know how to reproduce that.

@j
Copy link
Contributor

j commented Nov 19, 2019

@MichalLytek yup, that ended up fixing the error! 💪

@MichalLytek MichalLytek added Bugfix 🐛 🔨 PR that fixes a known bug and removed Bug 🐛 Something isn't working Solved ✔️ The issue has been solved labels Nov 21, 2019
@MichalLytek
Copy link
Owner Author

@j Thanks for confirmation!

Merging 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix 🐛 🔨 PR that fixes a known bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nested input types are not deserialized into class instances
2 participants