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

Annotation Based DefaultValues only works for strings #4220

Closed
1 task done
SleeplessOne1917 opened this issue Sep 10, 2021 · 6 comments · Fixed by #4226
Closed
1 task done

Annotation Based DefaultValues only works for strings #4220

SleeplessOne1917 opened this issue Sep 10, 2021 · 6 comments · Fixed by #4226
Labels
🐛 bug Something isn't working 🌶️ hot chocolate

Comments

@SleeplessOne1917
Copy link

SleeplessOne1917 commented Sep 10, 2021

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

The DefaultValue attribute only works for strings. Take the following class:

public class MyTypeInput
{
  [DefaultValue("foo")]
  public string String1 { get; set; }
  
  [DefaultValue(true)]
  public bool Bool1 { get; set; }
  
  [DefaultValue(42)]
  public int Int1 { get; set; }
  
  [DefaultValue(3.14)]
  public double Float1 { get; set; }

  [DefaultValue(MyEnum.Value1)]
  public MyEnum Enum1 { get; set; }
}

It produces the following SDL:

input MyTypeInput {
  string1: String = "foo"
  bool1: Boolean! = true
  int1: Int! = 42
  float1: Float! = 3.14
  enum1: MyEnum! = VALUE1
}

Note that the Boolean, Int, Float, and MyEnum fields are all non-nullable despite having default values defined. When making a request to the endpoint, the non-null requirement makes it so those fields have to be set in the input, treating them exactly the same as non-null fields without a default argument specified in 11.3.7. It recognizes the defaults in 12.0.0-rc.6.

I also tried this with the following class:

public class MyTypeInput
{
  [DefaultValue("foo")]
  public string String1 { get; set; }
  
  [DefaultValue(true)]
  public bool? Bool1 { get; set; }
  
  [DefaultValue(42)]
  public int? Int1 { get; set; }
  
  [DefaultValue(3.14)]
  public double? Float1 { get; set; }

  [DefaultValue(MyEnum.Value1)]
  public MyEnum? Enum1 { get; set; }
}

Note that this time, the bool, int, double, and MyEnum are all nullable. Nevertheless, the exact same SDL gets produced.

Update: I originally tested this in 11.3.7. I updated my examples to account for a bug fix that wasn't included in that version.

Steps to reproduce

  1. Clone this repository
  2. Build and run the solution
  3. Download the SDL at https://localhost:5001/graphql?sdl (exact url may differ on your machine)
  4. Note that the input type produced has fields that have default values marked as non-null

You can also attempt to hit the graphql endpoint with an editor of your choice. If you attempt to use the returnTypePassedIn query without specifying the bool, float, enum, and int fields, you'll get an error like "The field 'field name' is required and must be set." the query will work in 12.0.0-rc.6, but fail in 11.3.7.

Relevant log output

No response

Additional Context?

No response

Product

Hot Chocolate

Version

12.0.0-rc.6

@SleeplessOne1917
Copy link
Author

Update: I tested making a request using defaults in 12.0.0-rc.6 and it worked. It seems that the request only failed on 11.3.7. It's worth mentioning that the exact same SDL is produced in both versions, it just seems that 12.0.0-rc.6 interprets that SDL more loosely. If the generated SDL ever needs to be used outside of a Hot Chocolate project, it may be interpreted more strictly. It also shows errors for input in other GraphQL IDEs, even if the query is legal. For example, here's a screenshot of the popular IDE Altair showing errors for what is, for Hot Chocolate, valid input:
altair error

@michaelstaib
Copy link
Member

That has nothing todo with strict 😅 we just implement already the spec draft. The new spec release is slated for later this month. It will take a while for tools to update certain things. Hot chocolate 12 also already implements features of the very next spec draft like defer and stream.

@SleeplessOne1917
Copy link
Author

I read the 2021 spec and you're right: default values don't care if the type is non nullable. That raises an interesting question. Why were default values in version 11.3.7 of hot chocolate not working with non null types? Having checked the 2018 spec, it seems that it was valid to set a default value for a non null field/argument. That makes me think that default values working in version 12 but not 11.3.7 isn't a result of implementing the newest spec.

@michaelstaib
Copy link
Member

Because the 2018 spec had some issues with validation rules... I have to look them up again. But if you would implement the 2018 spec validation rules as described you would run into this... I need to check again my discussion notes with Ivan from GraphQL-js but there were contradicting rules.

@SleeplessOne1917
Copy link
Author

Interesting. Thanks for sating my curiosity.

On an unrelated note, I think Hot Chocolate is a great tool. Keep up the good work.

@michaelstaib
Copy link
Member

michaelstaib commented Sep 15, 2021

BTW there is a new draft on default values written by Benji that is not going into the 2021 spec draft. If you are interested in the odd cases. He is trying to root a lot of these cases out and streamline the handling of default values.

It is at RFC 2 and is slated for next years spec release.

#4179

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 🌶️ hot chocolate
Projects
None yet
2 participants