This project is read-only.

Authenticated Service Client

May 27, 2015 at 6:50 PM
I just commited some code, that makes the authenticated API working -- to some extend. I am not entirely happy with the solution, but I'm not finished with it either.

To make this happen, I changed the GW2 static class (no called GW2Bootstrapper) non-static and added a constructor and method that accepts an api key as string.
Should the user create the class with the default constructor, no Authenticated ServiceClient is created.
The user will always have access to the public part of the api, since the ServiceClient and the AuthorizedServiceClient are two different classes.

In a lengthy mail conversion Steven suggested, that we give the Service Client the key with each request, so the user can use multiple keys in one instance. I don't think this is a feasible solution.
First: Our users don't have access to the Request classes and opening them would be against the intention of making the API as easy to use as possible. Thus we would have to inject the key at the beginning and the user would have to specify the endpoint he wants the key to use for. We would then have to parse this information and inject the key in the right location.
This is some serious work we have to do, just for multiple keys in one instance.

The other solution would be to create a request with each key and try them subsequently. This however would mean multiple tries, where we don't know if they will succeed.

In my solution we don't have these problems. We create the instance with a key that is valid for the whole instance. If the user wants to use n-keys, he has to create n-instances of the GW2Bootstrapper. He also can change the key for one instance with the appropriate method on the Bootstrapper, although this will discard the old key entirely.
There is no need for the repository or the requests to have knowledge of the api key. The authentication is done by the service client and this class needs to know the key. This allows us to keep the current repository design intact.
This has benefits for future use too. Should the authentication change, for whatever reason, again. We only have to change a handful of classes at most. Would we incorporate the authentication in the requests and repositories we would have to change much more.


Steven also suggested, that his approach would allow us to read keys from the web.config or similar files.
This approach has nothing to do with the focus of our library. We don't care how the user stores his keys, or how many, or which scope they have. We accept them and then work with them.
May 27, 2015 at 8:50 PM
Edited May 27, 2015 at 8:52 PM
I don't like this, because the IServiceClient is supposed to be agnostic about what endpoint it's talking to. Knowing the details of the endpoint is the job of the IRequest. Requests are constructed by the IRepository, so the repository should also know about the API key. Any design that does not flow from these core principles, is just not right.

We don't have to change existing repositories to make this happen. Only new ones.
May 28, 2015 at 1:32 AM
I was going to write up a big reply with my brain dump of reasoning in either direction, but instead, in summary, I think both approaches have positives. Ruhrpottpatriot's approach means less code changes required, meaning potentially better maintainability, while Steven's makes a bit more sense to me from a "who should require the key" perspective... so I'm kind of torn. If there is a way of achieving Steven's approach without too much work required, that would be my suggestion.

For what it's worth, if I were to use an authenticated endpoint, my first-impression for how I would expect it to look (based on the existing api), would be:
var authenticationKey = "188A9F72-43D0-4208-9A33-2E61CB86EF77"; // or whatever the user provides
var accountsRepository = GW2.V2.Accounts.ForDefaultCulture();
var accountInfo = accountsRepository.GetInformation(authenticationKey);
I think this can be achieved regardless of which approach is taken.
May 28, 2015 at 11:06 AM
Edited May 28, 2015 at 11:31 AM
We can't use action injection like that. It doesn't integrate well with existing interfaces.

Imagine a bulk-expanded /v2/colors endpoint that requires an authentication header.

Existing implementations use this signature:
var colors = colorsRepository.FindAll();
So imagine that there is an endpoint /v2/accounts/colors that gets only the unlocked colors for your account.

The overloaded signature would look like this:
// works
var colors = accountColorsRepository.FindAll(key: "188A9F72-43D0-4208-9A33-2E61CB86EF77");

// HTTP 400
var colors = accountColorsRepository.FindAll();
The problem is that the other signature now has to throw an InvalidOperationException (or something like that)
May 28, 2015 at 11:47 AM
Using the example above, The Right Way™ to do it is by injecting the key in the constructor of the repository
class AccountColorsRepository
{
    private readonly string key;

    public AccountColorsRepository(string key)
    {
        this.key = key;
    }
}
That way, we can continue to use FindAll() and friends like we otherwise would
public IDictionaryRange<int, Color> FindAll()
{
    IRequest request = new AccountColorsBulkRequest
    {
        Authentication = "Bearer " + this.key,
        Culture = this.Culture
    };

    IResponse response = this.serviceClient.Send(request);
    return ...
}
In the Send() method of the IServiceClient, you would then copy the key from the IRequest to the HttpWebRequest.

I don't understand why you think this is too much work. I just described every change that we have to make in just a few paragraphs.
May 28, 2015 at 12:48 PM
Edited May 28, 2015 at 1:01 PM
I was thinking it would turn into a lot of work because you'd then have to modify IRequest?
May 28, 2015 at 2:42 PM
Yep we would have to write a complete new Inheritance hierarchy for authenticated requests. There is another problem. The current implementation of ServiceClient makes it impossible to fetch the key from the request, changing that just takes 2 min, but is very hack-ish and sloppy in my eyes. Also we have to at least add a check if this is an authorized request or not, to decide whether we want to add the header or not.

We could rewrite the Request classes, so that every Request has a "Key" property that defaults to string.Empty and we add the header every time, but that feels sloppy too.

In any way, the ServiceClient has to have at least some knowledge about the endpoint (authorized or not). Then why not make two implementations? One "public" and one "private", after all we know that if the user wants to access a certain endpoint he has to be authorized, no need to go the extra mile if we can achieve it much faster with less work. That might not be 100% sound for theorists, but we are not at university, are we?
May 29, 2015 at 12:21 PM
Ruhrpottpatriot wrote:
Yep we would have to write a complete new Inheritance hierarchy for authenticated requests.
Just put it on IRequest for all I care.
IRequest
{
    string Authentication { get; set; }
}
Repositories that don't need to authenticate can just ignore that property.

Ruhrpottpatriot wrote:
There is another problem. The current implementation of ServiceClient makes it impossible to fetch the key from the request, changing that just takes 2 min, but is very hack-ish and sloppy in my eyes.
How is this any different from getting the other values from the IRequest into the HttpWebRequest?

Ruhrpottpatriot wrote:
Also we have to at least add a check if this is an authorized request or not, to decide whether we want to add the header or not.
if (request.Authentication != null)
{
   // Add the header
}
Ruhrpottpatriot wrote:
We could rewrite the Request classes, so that every Request has a "Key" property that defaults to string.Empty and we add the header every time, but that feels sloppy too.
Alright, I'll give you that one. All request classes will have an Authentication property, even if it's never used. Big deal...

Ruhrpottpatriot wrote:
In any way, the ServiceClient has to have at least some knowledge about the endpoint (authorized or not).
NO. The service client is supposed to be a dumb abstraction over the HTTP transport layer. It should know how to set an authentication header, but it should not know or care about what the exact header value should be.
May 29, 2015 at 2:27 PM
To continue the discussion on this, I've created a couple shelvesets with some changes required if we were to follow Steven's proposal.
  • Auth - Method 1 - IRequest Modification
    • Includes modifications to IRequest itself. This means we have to update a ton of other classes, which isn't all that great imo, especially when most of them don't even use authentication.
  • Auth - Method 2 - IAuthRequest
    • Instead of modifying IRequest, this shelveset includes a new IAuthRequest. Much cleaner, if you don't mind that the ServiceClient casts the IRequest to an IAuthRequest (after checking if it is an IAuthRequest).
  • Auth - Method 2 - AuthRepositoryFactoryBase
    • Continuing with the IAuthRequest method, this includes changes to V2 Accounts as an example.
These aren't meant to be ready-to-check-in shelvesets, just examples to discuss.
May 29, 2015 at 3:02 PM
I really don't mind that there will be an unused property on a lot of request classes when we go with the first approach. Repositories that don't require authentication can just ignore that property. No header will be sent.
May 29, 2015 at 5:04 PM
How is this any different from getting the other values from the IRequest into the HttpWebRequest?
The method used to create the request has private HttpWebRequest CreateHttpWebRequest(Uri uri) as signature, so we build the request only with the URI. no other values are put into the request (since we only needed the uri up until now)
NO. The service client is supposed to be a dumb abstraction over the HTTP transport layer.
The check alone makes the ServiceClient know something about the request: It can be authorized or not, that is not totally agnostic.


After thinking about it for another day I think we should go another mile and change the Request interfaces and classes much more. Currently we have a ton of duplicate code, mostly when the request is localizable.
Currently we have:
  • BulkRequest
  • DetailsRequest
  • DiscoveryRequest
  • IBulkRequest
  • IDetailsRequest
  • IPageRequest
  • IRequest
In addition we have the requests for each node. Most of them are localizable. Currently we have the code in the lowermost class and thus we have tons of code redundancy. Right now I'm working on a revision of the inheritance hierarchy to eliminate the problem. With the current design we would get some more classes, but we would reduce redundancy by about 80%.
In the end not much changes, except this solution is much more close to Stevens solution than mine. However with this approach we have to rewrite the ServiceClient to make use of this, but I'm on that already.
Here is the class diagram:
http://imgur.com/hgz4Yrg


Thought on the new design?
May 29, 2015 at 5:56 PM
Edited May 29, 2015 at 5:57 PM
Better from a pure OO perspective, but annoying to implement. The Send() method will have to cast the IRequest to each interface to see if it needs additional request details.

I'd actually like to have a solution that implements HTTP/1.1 as defined by the official specification (RFC 7230) instead of something that was hacked together. Unfortunately, a pure implementation of the spec does not exist in .NET...

Maybe we should throw out IServiceClient / IRequest / IResponse and replace it with System.Net.Http.HttpClient?
May 29, 2015 at 6:08 PM
I'll think of a solution. I'll have a bit more time to do this today since I'm done with most uni stuff.
May 29, 2015 at 6:32 PM
One thing: Before we are doing this, or uploading any other source code, we should make the move to github.
May 29, 2015 at 7:32 PM
I read somewhere that the codeplex guys can move our code to git and preserve all history. Did I dream that up?
May 29, 2015 at 7:39 PM
StevenLiekens wrote:
I read somewhere that the codeplex guys can move our code to git and preserve all history. Did I dream that up?
Nope, it's legit. Quick google search came up with multiple articles on how to migrate from Codeplex (or just plain TFS) to GitHub with all history intact.

Example:
http://chriskirby.net/migrate-an-existing-project-from-tfs-to-github-with-changeset-history-intact/
May 30, 2015 at 10:15 AM
Edited May 30, 2015 at 10:21 AM
I thought that codeplex itself supports Git as a source control provider, and that the codeplex team can migrate your project from TFS to Git without ever leaving codeplex. At least that's how I remember it. I can't find the page where I read that any longer.

If that's still the case, we can switch to Git while everything is still on codeplex, then migrate the code, documentation and unresolved issues to GitHub on our own terms.
May 30, 2015 at 10:44 AM
May 30, 2015 at 12:09 PM
Regarding another unresolved issue: being able to parse the HTTP Link header for bulk-expanded page responses is also the responsibility of the HTTP client. WebRequest/WebResponse doesn't do this. I don't know any HTTP library that does.
May 30, 2015 at 12:56 PM
Edited May 30, 2015 at 12:57 PM
With HttpClient you have the possibility to look at the header without getting the content, so we'd have to write a parser against that.

On thing I forgot on top of that:
The Send() method will have to cast the IRequest to each interface to see if it needs additional request details.
You don't have to, since IRequest contains all methods needed to build a complete request.


//edit: I'll look into the conversion and see how and when to move to github.
May 30, 2015 at 3:45 PM
So I contacted the folks at Codeplex to convert our repository from TFS to Git, let's see when this is happening
Jul 21, 2015 at 11:24 AM
Welp... it's nearly two months later and I see no action from their end. I think it's safe to say that they have no interest in helping us move to a different provider. I guess we'll have to do this the hard way.

My plan is to handle migrating the codebase to GitHub myself, then transfer ownership to you.

Unresolved TFS work items should be copied to GitHub issues, but I doubt that this is possible to automate. I know that GitHub has an issues API, but is there any way to extract TFS work items in a reusable format?
Jul 21, 2015 at 12:28 PM
Edited Jul 21, 2015 at 12:29 PM
Sorry I haven't posted anything in the last two months. I contacted Codeplex three times now and I haven't got any response from them. That and I'm currently in my exam phase, which leaves me not much time to some a whole lot.

The Thing I found as help to migrate are these three things:
http://blog.benhall.me.uk/2009/12/migrating-from-codeplex-to-github/
https://github.com/objorke/CodePlexMigrationTools
http://stackoverflow.com/questions/26894037/automatically-migrate-codeplex-project-to-github

The first is pretty old, but should work regardless. As you can see at the last link, CP has no isses api to extract issues, so the answer to your question would be: no

I'll try to contact Codeplex one last time. My main issue why I haven't made the migration myself is, because I would like to keep the revision history (for obvious reasons).
Jul 21, 2015 at 1:36 PM
Edited Jul 21, 2015 at 1:46 PM
Take a look at git-tfs: https://github.com/git-tfs/git-tfs

This tool should make it possible to convert the repository while preserving history.

There might be a way to extract TFS work items using team explorer APIs.
https://msdn.microsoft.com/en-us/library/microsoft.teamfoundation.workitemtracking.client.aspx

Ruhrpottpatriot wrote:
The Thing I found as help to migrate are these three things:
http://blog.benhall.me.uk/2009/12/migrating-from-codeplex-to-github/
https://github.com/objorke/CodePlexMigrationTools
http://stackoverflow.com/questions/26894037/automatically-migrate-codeplex-project-to-github
Actually I didn't know that GitHub already has a migration tool. Have you tried it? It looks promising.
Jul 23, 2015 at 5:07 PM
So they converted our repository to git. I'll move it to Github as soon as possible, probably even today.
Jul 24, 2015 at 2:56 PM
Little update: The main repository is over at github, But I need to make some changes, since branches and the like where not converted properly. Also Work items do not get carried over.
Jul 26, 2015 at 11:58 AM
What did they do wrong?
Jul 27, 2015 at 9:40 PM
Edited Jul 27, 2015 at 9:40 PM
Nothing apparently. Branches are now different folders if I see it correctly. And Github only imports the repository, no work items etc. since there is no API for that.
I'm currently testing something out on my local machine, but I am no expert in git rebase so it is a bit of trial and error.
Jul 28, 2015 at 8:45 AM
You mean that there is only a single git branch? And that branch contains one directory for every tfs branch? Oh crap... that's not good.

2015-07-27 22:40 GMT+02:00 Ruhrpottpatriot <[email removed]>:

From: Ruhrpottpatriot

Nothing apparently. Branches are now different folders if I see it correctly. And Github only imports the repository, no work items etc. since there is no API for that.

Read the full discussion online.

To add a post to this discussion, reply to this email ([email removed])

To start a new discussion for this project, email [email removed]

You are receiving this email because you subscribed to this discussion on CodePlex. You can unsubscribe or change your settings on codePlex.com.

Please note: Images and attachments will be removed from emails. Any posts to this discussion will also be available online at codeplex.com


Jul 28, 2015 at 6:32 PM
Edited Jul 28, 2015 at 6:34 PM
Look at the codeplex sources tab. That is the structure of the git repository. On github it won't look any different.

Basically what I no want to do is:
  • Clone the repository.
  • Create branches locally
  • Create tags for each release
Sep 12, 2015 at 11:39 AM
This is now done.

Repository: https://github.com/StevenLiekens/GW2.NET
Releases: https://github.com/StevenLiekens/GW2.NET/releases

TODO: transfer ownership back to you
TODO: copy release notes to github