Describe the bug
In 0.7.0-preview33 - When submitting an enum value as anything other than all-caps, the parser fails to recognize the value as a legitimate enum value.
To Reproduce
With the following unit test added in EnumTypeTests.cs
[Fact]
public void ParseLiteralIgnoresCase()
{
// act
Schema schema = Schema.Create(c =>
{
c.RegisterType(new EnumType<Foo>());
c.Options.StrictValidation = false;
});
var enumValue = new EnumValueNode("Bar1");
// assert
EnumType type = schema.GetType<EnumType>("Foo");
var value = type.ParseLiteral(enumValue);
Assert.True(true);
}
the bug can be seen.
Expected behavior
Enum values should be able to be parsed when they match the Enum value, as well as matching the schema description. Ideally we would ignore case when parsing enum values
Additional context
Since the implementation of EnumType uses a dictionary of strings to Enum Values, there is no way to ignore case universally when parsing literal. I see three ways to potentially fix this:
Use a ToUpper() on each value before adding to the dictionary or trying to access the dictionary.
I personally don't like this solution because it adds a lot of extra code in several different places, and forgetting to add it in a new place would reintroduce the bug.
Add all permutations of the Enum values to the dictionary on initialize: ie, loop through the values, add their descriptions, add the values string representations, add those string representations ToUpper and ToLower.
This is better because all of the string manipulation would happen in the initialize method, so at least its only in one place. However, there is no way to know that we have captured every permutation someone might enter, but maybe that's acceptable. If someone enters bAr1 as their enum value, maybe it's okay to just fail.
Stop using a dictionary for this and just use the Enum.Parse method, which allows us to ignore case.
This requires the most code to be changed, but is probably the most elegant solution.
btw. the dictionary class allows to specify a comparer in the constructor
That's a good idea, I didn't realize you could do that.
The GraphQL enum type does not necessarily have an enum as value type. You could have other types as .net representations.
Moreover, the spec recommends to use all caps for enum names.
Enum values are represented as unquoted names (ex. MOBILE_WEB). It is recommended that Enum values be “all caps”. Enum values are only used in contexts where the precise enumeration type is known. Therefore it’s not necessary to supply an enumeration type name in the literal.
2018 Spec Links
Enum Value
Enum Type
Furthermore, if the API specifies that the name is all caps then the enum value should be send all caps.
In c# the enum value Abc is different from the enum value ABC, so you could create an enum.
public enum Foo
{
BAR
bar
}
So, I am not really sold yet on changing the behavior.
So, the second part about the coercion rules sound to me that the enum type is as set of precise constant values.
You can still opt out of the all caps naming with the descriptor. Moreover, with version 0.8.0 we will add apis that let you change the default naming.
I have moved this issue to the 0.8.0 backlog in order to discuss this further for the next major release.
In my opinion it is very risky to see "Abc" as 1 thing and "abc" as another. I always make my APIs ignore the casing of enum values - hence I am all for making this change. I think it produces more robust solutions.
The issue here is that GraphQL and C# are both case-insensitive so you cannot just convert it. It might be ok to do that in a specific project but for a library this can lead to frustrations for the user.