[WIP] Auditing the GitHub API using Analyzers

[shiftkey]

I found myself working through the excellent work that @naveensrinivasan had done to parse the GitHub API docs and identify which endpoints had not been implemented in Octokit.

octokit/octokit.net#968 (comment)

And sure, I could audit these manually - in fact I started doing this, but then I paused to think about how I could automate this, so that it could be done more than once.

This PR starts the first step of creating that auditing process. Let me explain what I'm trying to do here...

The Current State Of Affairs

Let's say we have a method that does this:

public Task<IReadOnlyList<string>> GetAllForRepository(string owner, string name)
{
    Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
    Ensure.ArgumentNotNullOrEmptyString(name, "name");

    return ApiConnection.GetAll<string>(ApiUrls.IssuesEvent(owner, name));
}

It's not easy to see what resource this is actually associated with - I need to navigate to ApiUrls and look at the implementation:

public static Uri IssuesEvent(string owner, string name)
{
     return "repos/{0}/{1}/issues/events".FormatUri(owner, name);
}

I also need some familiarity with the GitHub API docs to ensure that this is actually the right format (arguments are added, in the correct order).

This is also just a convenience method. I'm not sure it adds much these days.

The Problem

Let's say I have a page of resources that I extracted from the GitHub API docs:

/activity/events/

 - GET /events
 - GET /repos/:owner/:repo/events
 - GET /repos/:owner/:repo/issues/events
 - GET /networks/:owner/:repo/events
 - GET /orgs/:org/events
 - GET /users/:username/received_events
 - GET /users/:username/received_events/public
 - GET /users/:username/events
 - GET /users/:username/events/public
 - GET /users/:username/events/orgs/:org

This is just the interesting bits after you parse this page:

https://developer.github.com/v3/activity/events/

If I wanted to be in sync with the GitHub API, I'd like for all of these actions to exist on one of the Clients defined here - but how could I verify this programmatically?

An Alternative Proposal

Broadly speaking, there's two things I'd like to verify:

• for a documented feature (e.g. /activity/events/) I'd like to verify that a number of methods are implemented
• for each method, verify that it's actually invoking that resource

So let's 🔥 the ApiUrls class to the ground as that's getting in the way, and move these resources closer to where they're needed.

I'm imagining something like this for defining the contract:

[Endpoint("repos/:owner/:repo/issues/events")]
public Task<IReadOnlyList<string>> GetAllForRepository(string owner, string name)
{
    Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
    Ensure.ArgumentNotNullOrEmptyString(name, "name");

    var uri = "repos/{0}/{1}/issues/events".FormatUri(owner, name);

    return ApiConnection.GetAll<string>(uri);
}

For other scenarios, you may want to specify the verb as well:

[Endpoint("GET", "repos/:owner/:repo/issues/events")]
public Task<IReadOnlyList<string>> GetAllForRepository(string owner, string name)
{
    Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
    Ensure.ArgumentNotNullOrEmptyString(name, "name");

    var uri = "repos/{0}/{1}/issues/events".FormatUri(owner, name);

    return ApiConnection.GetAll<string>(uri);
}

Benefits:

• can compare attribute and defined endpoint at compile time
• can inspect ApiConnection usages matches documented behaviour
• can use reflection to verify API coverage after build (not done here)
• can be introduced gradually by opt-in
• becomes an IDE integration available to anyone on VS2015

Problems:

• duplication of resources?
• general inexperience with Roslyn interfering with the contributing experience

To Do List:

• actually implement a failing test
• fail the test if the parameters don't match
• fail the test if the parameters are out of order
• setup a scenario to use string interpolation so we can test that too
• get some feedback from people more experienced with Roslyn

cc @Pilchie @haacked @M-Zuber for general feedback

[Pilchie]

General feedback:

1. This looks to have not much to do with Roslyn 😄
2. FormatUri type methods makes me want to poke my eyes out - use string interpolation instead (but be careful about what culture you use, by target typing to FormattableString.
3. If you're willing to do reflection in the method body why not extract the resource from the attribute instead of duplicating it: I'm thinking of something like (pseudocode, do not try to compile):

private Uri GetResource([CallerMemberName] string method = null)
{
    var methodInfo = typeof(this).GetMembers(method).GetCustomAttributes().Single(a => a.Name == "EndPoint").ExtractConstructorArgumentHere();
}

(if you have overloads, this gets tricker of course

[shiftkey]

@Pilchie thanks for the 👀

1. Fair point, and perhaps it's because I'm just dipping my toe in. I initially thought about using something like Fody to rewrite the IL, but I keep not finding that right reason to use Roslyn in anger. I'm softly moving Octokit into this whole "Roslyn" world, and analyzers are something that I'd really like to understand beyond sample code, so I'm going to soldier on anyway for the sake of this thought experiment.
2. We're not using C# 6 yet in Octokit - that's why we've had these FormatUri type methods around. Moving the codebase to C# 6 and using those should make verifying the string contents more interesting. But let's cross that bridge later.
3. I thought about that too, but I avoided that because you don't really have a chance to verify it's the right format before runtime (unless I use Fody and make it break the build) - hence me hacking away at Roslyn.

[shiftkey]

@Pilchie and I apologise in advance as I'm probably mis-purposing the "Roslyn" name here for the various components of the stack 😁

[Pilchie]

Ok - looking at your commits I understand a bit more what you're trying to do, and it could make sense as a Roslyn analyzer.

I was actually thinking that you could steal a page from MVC, and pass an anonymous type with all the parameters into the method, and it could do the substitutions and verify that they match too:

private Uri GetResource(object params, [CallerMemberName]method = null)
{
    // ...
}

// Note the change from "name" to "repo" so that it matches the attribute.
[Endpoint("GET", "repos/:owner/:repo/issues/events")]
public Task<IReadOnlyList<string>> GetAllForRepository(string owner, string repo)
{
    Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
    Ensure.ArgumentNotNullOrEmptyString(repo, "repo");

    var uri = GetResource(new { owner, repo });

    return ApiConnection.GetAll<string>(uri);
}

(Note, when you go to C# 6, you can also use nameof on all those Ensure calls ✨ )

[shiftkey]

@Pilchie that's interesting, still a runtime concern but avoids the need for an analyzer completely..

[M-Zuber]

💯 I like it

My assumtion is that you will then use reflection to get every method with the endpoint attribute to match it up to the offical docs?

[naveensrinivasan]

@shiftkey 👍 This was exactly what I had in mind and parsing the API was step one.

[haacked]

still a runtime concern but avoids the need for an analyzer completely..

What do you mean here? Do you mean that

can compare attribute and defined endpoint at compile time

is not enabled? It seems to me that the approach @Pilchie proposes completely obviates the need to do any checking because the thing we're checking against is the thing that supplies the URL. Why have compile time checking if we can completely remove the need for it.

We might even be able to avoid reflection by using a partial class and have Roslyn generate the method to generate the URL based on the attribute.

It's funny, because at the MVP summit, Miguel, Mads, and I (among others) talked about a desire for some partial code generation (aka Macros). This seems like a nice potential application of such a thing. 😄

[Pilchie]

@haacked see https://github.com/mattwar/roslyn/tree/Injectors for a prototype of the code injector thing.

[shiftkey]

@haacked

It seems to me that the approach @Pilchie proposes completely obviates the need to do any checking because the thing we're checking against is the thing that supplies the URL.

The biggest reason why I'm looking at this approach over reflection (in whatever form) is that we can verify things at compile time. Basically, I'd like to know before executing the code that things are correct (or not correct).

We might even be able to avoid reflection by using a partial class and have Roslyn generate the method to generate the URL based on the attribute.

This definitely intrigues me.

[haacked]

Basically, I'd like to know before executing the code that things are correct (or not correct).

Walk me through this if you don't mind. What kind of things are you checking to see are correct or not via using an attribute and then using it again in the code?

[shiftkey]

@haacked sure!

Things to check at compile time:

• the URL used in the method matches the resource declared in the attribute
• the parameters provided in code satisfy the necessary placeholders
• [nice to have] not out of order, variable names match, etc

Things to check post-build (reflect over assembly):

• Client locations match documentation structure e.g. /activity/events is found at github.Activity.Events
• Clients implement expected resources from documentation

[shiftkey]

One potential issue with using reflection APIs at runtime "like MVC" is their availability on other platforms like .NET Core and PCL targets:

[haacked]

There's a simple trick to this. In .NET Core/PCL, you have to call GetTypeInfo to call the GetCustomAttributes methods.

type.GetTypeInfo().GetCustomAttribute<WrapperObjectAttribute>();

So I typically write an extension method named GetTypeInfo for System.Type that just returns the passed in type and then conditionally compile it out of CoreCLR etc. Then I use it everywhere.

#IF NOT CORECLR
public static Type GetTypeInfo(this Type type) { return type; }
#ENDIF

Also, it looks like GetCustomAttributes is available on core clr.

As far as I know, all the reflection APIs except reflection emit are available on every platform. There's some small differences.

Hopefully, for the types of things we need to do, we could rely on loading attributes in a reflection only context using GetCustomAttributesData instead of GetCustomAttributes

[naveensrinivasan]

Why not use https://github.com/jbevain/cecil? I am sure this should work in coreclr. I was thinking of doing a PR using cecil.

[shiftkey]

@haacked oh! I guess that's one less issue for me to worry about!

[M-Zuber]

How does the work in octokit/octokit.net#1290 fit into this? (if at all)

[shiftkey]

@M-Zuber 1290 makes our usages of URIs more consistent, and we could now audit the ApiUrl classes directly (using reflection and passing in dummy data) to then verify the format is correct, and know we've covered all our usages...

[M-Zuber]

okay, but aren't you suggesting here to 🔥 the ApiUrl class and move the urls back into the higher level methods?

[shiftkey]

@M-Zuber maybe, but I think that's a "nice to have" thing rather than essential...