Hotchocolate: [Feature Discussion] Graphql Documentation Generation

[John0x]

Summary

Currently the XML Documentation is used to automatically generate graphql schema documentation, which already is pretty handy 👍
Though it can be improved in my opinion.
I personally like to include the errors/exceptions that can be thrown by the mutation/query in the documentation so that someone who consumes the API already knows what error codes he should handle.

The XML Documentation feature gives us a lot of handy elements that could be utilized to generate a great interactive documentation.
Elements that I'm interested in right now:

• exception
• returns
• example

I'm willing to implement a more detailed documentation generation, but I might need some assistance since it would be my first hc contribution 🙂

Any thoughts on this?
Especially on how to format it and which elements should be converted.

Proposal

Current State

        /// <summary>
        /// Updates the user
        /// </summary>
        /// <param name="email">Email of the user</param>
        /// <param name="input">User DTO</param>
        /// <returns>User object of the updated user</returns>
        /// <exception cref="QueryException" code="USER_NOT_FOUND">Couldn't find user</exception>
        /// <exception cref="QueryException" code="DB_NOT_NULL_VIOLATION">General database not null violation</exception>
        /// <exception cref="QueryException" code="DB_UNIQUE_VIOLATION">General database unique violation</exception>
        /// <exception cref="QueryException" code="DB_UPDATE_EXCEPTION">General database update exception</exception>
        public async Task<User> UpdateUser(string email, UpdateUserInput input)
        {
            return await _userService.UpdateUser(email, input);
        }

Gets generated to:

  """
  Updates the user
  """
  updateUser(
    "Email of the user"
    email: String!
    "User DTO"
    input: UpdateUserInput!
  ): User!

Suggested State

Should get generated to:

  """
  Updates the user

  **Returns:** User object of the updated user

  **Errors:**
  1. USER_NOT_FOUND: Couldn't find user
  2. DB_NOT_NULL_VIOLATION: General database not null violation
  3. DB_UNIQUE_VIOLATION: General database unique violation
  4. DB_UPDATE_EXCEPTION: General database update exception
  """
  updateUser(
    "Email of the user"
    email: String!
    "User DTO"
    input: UpdateUserInput!
  ): User!

Links

Previous discussions:

• https://hotchocolategraphql.slack.com/archives/CD9TNKT8T/p1582119782301400
• https://hotchocolategraphql.slack.com/archives/CPBHBBN12/p1583418183025000

Relevant documentation:

• https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/xmldoc/recommended-tags-for-documentation-comments

Relevant files:

• https://github.com/ChilliCream/hotchocolate/blob/master/src/HotChocolate/Core/src/Types/Types/Descriptors/Conventions/XmlDocumentationProvider.cs

[michaelstaib]

Old:

public interface IDocumentationProvider
{
    string GetSummary(Type type);
    string GetSummary(MemberInfo member);
    string GetSummary(ParameterInfo parameter);
}

New:

public interface IDocumentationProvider
{
    string GetDescription(Type type);
    string GetDescription(MemberInfo member);
    string GetDescription(ParameterInfo parameter);
}

I think we should refactor the documentation provider. When we talk about documentation in the context of GraphQL we call it description. So we should rename that to GetDescription and GetDescription will extract the description from a .NET type, member, parameter.

With this it is more clear that we actually do not only mean the summary.

We also should add to our interface some documentation and make that clear and also point to the fact that the string that is returned is allowed to use markdown.

Further, I like the proposal of your markdown structure for field documentation lets go ahead with that.

I think refactor the interface and go ahead with your proposal.

[rstaib]

Shouldn't we use GraphQL Descriptions for documentation purposes?

"""
My Type ;-)
"""

[John0x]

I agree @michaelstaib

Should we only add exceptions to the graphql docs that contain a "code" attribute or just all of them?

@rstaib
You mean directly in the c# code?
The xml docs get converted to graphql descriptions as far as I know. I just copied the examples from the playground schema, that's why they are comments in the examples and not descriptions.

[michaelstaib]

@John0x I think he means in your example of the sdl.

  """
  Updates the user

  **Returns:** User object of the updated user

  **Errors:**
  1. USER_NOT_FOUND: Couldn't find user
  2. DB_NOT_NULL_VIOLATION: General database not null violation
  3. DB_UNIQUE_VIOLATION: General database unique violation
  4. DB_UPDATE_EXCEPTION: General database update exception
  """
  updateUser(
    "Email of the user"
    email: String!
    "User DTO"
    input: UpdateUserInput!
  ): User!

since # in graphql are for comments. But you do not need to worry about that since these representations are correctly handled by the schema syntax serializer.

But for consistency we should correct the examples.

[michaelstaib]

@John0x we should only include the one with a code attribute since the other exceptions could be internal exceptions that you do not want to be exposed over your service layer.

[John0x]

Okay :)
I've updated the examples.

Attached is an image of the current status, any suggestions for the formatting or is it okay like that?
Generated from:

        /// <summary>
        /// Creates a review for a given Star Wars episode.
        /// </summary>
        /// <param name="episode">The episode to review.</param>
        /// <param name="review">The review.</param>
        /// <param name="eventSender">The event sending service.</param>
        /// <exception cref="Exception" code="TEST_ERROR">Test exception dude</exception>
        /// <returns>The created review.</returns>

[michaelstaib]

looks good so far

[rstaib]

looks nice

[rstaib]

One remark here regarding the exceptions, shouldn't we use an unordered list instead of an ordered list?

[John0x]

@rstaib generally yes, but the Playground doesn't seem to display unordered lists properly

[John0x]

Formatting with unordered list:

[rstaib]

@John0x but isn't this a problem inside the tool. The question is whether we should be semantically correct or working fine in a certain tool. Perhaps we should tell them. Have you tested Banana Cake Pop ;-) would be interested whether it's working fine.

[John0x]

@rstaib Generally I would agree with you, but the playground is the standard and comes preinstalled so it should at least look decent in there. An unordered list would semantically make more sense but at least for me, it's not too much of a problem if it was an ordered list. We should probably tell them, haven't done so yet though 😄

I'm gonna test it with the banana