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(discriminator): union nullable #2982

Closed
wants to merge 3 commits into from

Conversation

karatakis
Copy link
Contributor

Summary:
See the bug report #2981

Issue Reference(s):
Fixes #2981

Build & Testing:

  • I ran cargo test successfully.
  • I have run ./lint.sh --mode=fix to fix all linting issues raised by ./lint.sh --mode=check.

Checklist:

  • I have added relevant unit & integration tests.
  • I have updated the documentation accordingly.
  • I have performed a self-review of my code.
  • PR follows the naming convention of <type>(<optional scope>): <title>

@github-actions github-actions bot added the type: fix Iterations on existing features or infrastructure. label Oct 9, 2024
Copy link

codecov bot commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.56%. Comparing base (3bdf43b) to head (84c8e8b).
Report is 12 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2982   +/-   ##
=======================================
  Coverage   87.56%   87.56%           
=======================================
  Files         275      275           
  Lines       27253    27259    +6     
=======================================
+ Hits        23863    23870    +7     
+ Misses       3390     3389    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

let's update the test with list case as well

type Query {
  node: Node @expr(body: null)
  nodes: [Node] @expr(body: [{id: 1, username: "user"}, null, {id: 2, slug: "page"}])
}

@@ -238,7 +244,7 @@ impl Discriminator {

pub fn resolve_type(&self, value: &Value) -> Result<&str> {
let Value::Object(obj) = value else {
bail!("Value expected to be object");
return Ok(&self.name);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it could hide some errors when we are trying to resolve object type but getting something else. Null is a special case though, what about return Option<&str> here and for null use None?

@karatakis
Copy link
Contributor Author

Closing because it is covered from the refactor here #3000

@karatakis karatakis closed this Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: fix Iterations on existing features or infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When I query optional Union type I get error
2 participants