Hotchocolate: Generate schema descriptions from XML for enum elements.

[michaelstaib]

The conventions are pulling in xml documentations for types and members if available. We should add support for enum elements as well to have a consistent experience.

[michaelstaib]

715

[willwolfram18]

This should actually already supported as of PR #721 as it included a new method, GetEnumValueDescription() to the INamingConventions interface. The EnumValueDescriptor was then updated to use this new method. The EnumTypeDescriptor already had support for doing the INamingConventions.GetTypeDescription().

[michaelstaib]

I will write a test for this one.

[willwolfram18]

Looking at the snippet for GetEnumValueDescription() I linked, it would be worthwhile to make an update to verify that the enumType checks that it IsEnum. However, I'm uncertain of what the expected behavior should be if someone were to pass in a non-enum type value. Perhaps an InvalidOperationException because it's expecting an enum value?

[michaelstaib]

Not really, in schema-first an enum value can be basically everything and also in code-first there are probably use cases where you do not want to have a enum backing type.

So, we should be tolerant about the value. So, if the value is not an enum but a string for instance we cannot infer a description and just return null.

[willwolfram18]

As I was working on #753 I discovered that GraphQLDescriptionAttribute is not able to be placed on AttributeTargets.Enum which prevents using the attribute on the enum type itself. Supporting the enum values would require adding AttributeTargets.Field, but I'm not sure if that would cause issues if a user were to place that on a field for a GraphQL type.

[michaelstaib]

It should not case an issue I think. We should add this target. Awesome work with the documentation btw. Really appreciate your contributions 👍

[michaelstaib]

@willwolfram18 do you think we get this into the 9.0 release. we want to start building rc builds by the end of this week.

[willwolfram18]

I think it's possible to get it done and have RC builds going this week. I'll start chipping away at it 😸

[willwolfram18]

Just to clarify what all do we need here? Here is what I understand at this point

• [x] XML doc strings generating descriptions in the schma
  
  
  
  • Will include unit tests to keep this running correctly for future builds
  
  
• [ ] Update GraphQLDescriptionAttribute to support enums and their values

Do we need to support your suggested scenario of a code-first implementation that does not use enums as the backing type? If so, is there a sample of such a scenario I could build from?

[michaelstaib]

I have moved this issue to he V9 release.

I will have a look at the GetEnumValueDescription() at the moment this is causing the stitching layer to fail.

There is another thing I am pondering at the moment. If we use the non-generic types do we get the xml documentation that sits on the type classes? What do you think?

[willwolfram18]

I haven't tried XML docs with the non-generic types and I'm not immediately familiar with how they work but I'm willing to take a look and see what they're behavior is!

Because I don't know how they work I can't yet provide an informed opinion on how the documentation should work, but after I try it out I'll let you know my thoughts

[willwolfram18]

Alright did some tinkering with the EnumType class and found the following behavior given the following code snippet

public class EpisodeType
    : EnumType
{
    protected override void Configure(IEnumTypeDescriptor descriptor)
    {
        descriptor.Name("EpisodeNonGeneric");
        descriptor.Value(Episode.NewHope);
        descriptor.Value(Episode.Jedi);
        descriptor.Value("EMPIRE");

        base.Configure(descriptor);
    }
}

The above snippet will take the specified values of Episode.NewHope and Episode.Jedi and automatically pulls the XML doc strings. In the case of the "EMPIRE" string, the description is null. This is because the value's type, System.String, contains no "EMPIRE" member, and therefore it does not have a MemberInfo object to read from. 🎉

Now, where it would break down is if the enum value specified is an actual member on the type, such as using a value of "ToString". This is a member that can be found on the System.String type and has multiple implementations, which means the call to SingleOrDefault() will throw an exception due to finding too many matching MemberInfos. Unfortunately with the current implementation the error message is unhelpful ("Sequence contains more than one element.") so I am tempted to include a fix for this that catches the exception and throws a more user friendly one (or at least more descriptive one).

[michaelstaib]

https://github.com/ChilliCream/hotchocolate/pull/772/commits/9e5a64e8836d6c531916ce18daec339d43b02728#diff-ea2064a03bc4cdae875e0b92f49e5789

[michaelstaib]

The enum that you have there is fine. Since a GraphQL enum has not to allign with a csharp enum type. so, no error should be thrown. I have fixed the behavior now and included tests.

As I mentioned, with schema stitching you have no native enum types. Also with some schema-first scenarios we will run into the same issue.

[michaelstaib]

Type enumType = value.GetType();
            if (enumType.IsEnum)
            {
                MemberInfo enumMember = enumType
                    .GetMember(value.ToString())
                    .FirstOrDefault();

                if (enumMember != null)
                {
                    string description = enumMember.GetGraphQLDescription();
                    if (string.IsNullOrEmpty(description))
                    {
                        return _documentation.GetSummary(enumMember);
                    }
                    return description;
                }
            }

[michaelstaib]

So now we first check if the type is an enum and only if the type is an enum we will check for xml documentation.

[michaelstaib]

I have also thought about the other thing. So, basically if you have a non-generic type

public class Foo
    : ObjectType
{
}

In this case we will not use the xml documentation. I mean you will have to specify the name and so on on fluent, so why would we want to pull in xml documentation in this case?

It would feel strange to use descriptors for everything but then use the xml documentation just for description.

It alines also with the generic type. In case of the generic object type ObjectType<T> we are including the documentation fromTbut not formObjectType`.

[michaelstaib]

I will close this one since we fixed it with preview 38.

[willwolfram18]

These changes and suggestions definitely make a ton of sense! Glad to see they have been fixed!