Gallery Server Abstraction

Jan 4, 2011 at 11:56 PM
Edited Jan 5, 2011 at 4:57 PM

The idea here is to create an abstraction of the Gallery Server so that it can be programatically called without having to deal with newing up an HTTP client each time. This would make it very simple to interact with the Gallery Server. Bellow is a list of "actions" that can be invoked via REST:

PackageService 

  • GetCollection()
  • Create(key, package) (Old API Will be Deleted)
  • Get(Key, Id, Version) 
  • Update(key, Id, Version, package)
  • Delete(key, Id, Version)
  • RatePackage(Id, Version, Rating)


PackageFileService

  • Create(Key, FileExtention, Stream) 
  • CreateFromExternalUrl(Key, FileExtension, Url)

PackagePublishService

  • Publish(Key, Id, Version)

ScreenshotService

  • Delete(Id)

Here is some example code of what I thought it would look like:

 

var server = new GalleryServer("my-key-123"); //Default Gallery Server
 -or-
var server = new GalleryServer("my-key-123", "http://nuget.osbornm.com");

if(server.CreatePackage("nupkg", pkgStream)){
 Console.Write("Package Created");
 if(server.PublishPacage("MyPackage", "1.0"){
  Console.Write("Package Published");
 } else {
  Console.Write("Package Created but not published");
 }
} 

A few things to note and questions:

  • True/False is returned unless the call is to get something in which case the JSON could be deserialized and return a object.
  • The key is added to the server object because only one API call doesn't use it. What happens if you only want to use the rating API?
  • Should we provide more access to the response that was sent for the server? In case the consumer wanted to find out why something didn't work? 
    • Should we return the response and leave it up to the consumer to understand the HTTP Status code. (I think this starts breaking the idea behind the abstraction)
  • What assembly should this code go into? Core? 
Coordinator
Jan 5, 2011 at 12:51 AM
  • What use is returning false? These operations should always succeed except in exceptional cases, right? I'd probably make them return void and throw an exception if they fail.
  • As for which assembly, is this going to be used by NuGet.Core?
Jan 5, 2011 at 12:57 AM
haacked wrote:
  • What use is returning false? These operations should always succeed except in exceptional cases, right? I'd probably make them return void and throw an exception if they fail.
  • As for which assembly, is this going to be used by NuGet.Core?

Well it might not always be "exceptional" cases when it fails. I don't know if having the key not be able to change a package is exceptional... Although I'm not opposed to making it throw exceptions. 

Well this would be Used by Nuget.CommandLine and anyone else that wants to interact with the Gallery Server. I could be used by an developer in say there CI build scripts to talk to the Gallery Server and push new bits. 

Jan 5, 2011 at 1:31 AM

Yes, throwing exceptions is best.

And yes, this should be in core.  Now watch Fowler complain about that.  He's very protective of core :)

Developer
Jan 5, 2011 at 2:56 AM
Edited Jan 5, 2011 at 3:08 AM

We should create a new assembly with all of the pieces that need to be shared by the clients. NuGet.Client, it'll come in handy later when we start adding settings files and other core things that need to be shared between vs and the command line. 

But maybe this thing should go into core if it's going to be the official nuget protocol for pushing packages to a gallery instance.

Coordinator
Jan 5, 2011 at 3:19 AM
Right on cue! And I agree. :)
Jan 5, 2011 at 3:28 AM
So, I just need to respond to this thread to get this off my chest.
It seems strange that the client abstraction of the nuget repository protocol is in the project but the server bits are in the gallery source code. For some reason that seems very odd, in fact it gives me heartburn. It seems like the protocol for the server bits should be part of nuget core or this client abstraction should be part of the gallery project. The Gallery project could handle the data management and other aspects, but having the implementation of the protocol split across two different oss projects. It seems like the client abstraction around the protocol should live in the same project/VCS as the server bits. Am I just not getting it ?
Jan 5, 2011 at 4:06 AM

I think Core is the right place for this right now. I don't know that it will ever be the "official protocol" it is meant to be an alternative to the "raw" REST API exposed around Gallery Server. 

Jan 5, 2011 at 7:15 AM

Eric has a point that since this API needs to be in sync with the gallery server, they would ideally be close together. Though doing that would probably add some pain on the NuGet side, since we need to consume this API is our nuget.exe, so we would need to get a binary from a different project.

As far as this being the 'official' protocol: the REST API is the official protocol.  The client APIs we're discussing here are just one convenient implementation of this protocol.  This is not much different from OData being our feed protocol, and NuGet.Core.dll containing one helpful client implementation for that protocol.

Jan 5, 2011 at 7:33 PM

Okay so I implemented a couple of the APIs that were used by the push command (in the Nuget commandline) so see what some code would look like. Compare the code below with the code that in the Push Command:

	public void Execute() {
            //Frist argument should be the package
            _packagePath = Arguments[0];
            //Second argument should be the API Key
            _apiKey = Arguments[1];

            var client = new HttpClient();

            if (String.IsNullOrEmpty(Source)) {
                throw new CommandLineException(NuGetResources.PushCommandNoSourceError);
            }

            var gallery = new GalleryServer(Source);
            ZipPackage pkg = new ZipPackage(_packagePath);

            Console.WriteLine(NuGetResources.PushCommandCreatingPackage, pkg.Id, pkg.Version);

            try {
                using (Stream pkgStream = pkg.GetStream()) {
                    gallery.CreatePackage(_apiKey, pkgStream);
                }
                Console.WriteLine(NuGetResources.PushCommandPackageCreated);
                if (Publish) {
                    Console.WriteLine(NuGetResources.PushCommandPublishingPackage, pkg.Id, pkg.Version.ToString());
                    gallery.PublishPackage(_apiKey, pkg.Id, pkg.Version.ToString());
                    Console.WriteLine(NuGetResources.PushCommandPackagePublished);
                }
            }
            catch (WebException e) {
                string errorMessage = String.Empty;
                var response = (HttpWebResponse)e.Response;
                using (var stream = response.GetResponseStream()) {
                    errorMessage = stream.ReadToEnd();
                }

                throw new CommandLineException(NuGetResources.PushCommandInvalidResponse, response.StatusCode, errorMessage);
            }
        }

 

Jan 5, 2011 at 8:23 PM
Edited Jan 5, 2011 at 8:23 PM

One problem with the code above is that the consumer is responsible for knowing to read the response to get a better contained in the WebException to get a meaningful error message. I think it would be better if we threw a new WebException with a meaningful error, aka the one actually returned by the Gallery Server.

So you would get

The uploaded file format was not valid

instead of (default error when there is a 400 level response)

The remote server returned an error: (400) Bad Request.

I would still preserve the response and status, this would basically just change the error message to something helpful by default. Thoughts? 

Jan 5, 2011 at 8:36 PM

Sure, you can try using a new exception.  But make sure you pass the original one as the inner exception, to make sure that nothing is lost.

Other comment: weren't you going to pass the key in the ctor instead of into each API?

Developer
Jan 5, 2011 at 9:20 PM

The reason it can be bad to have too many convenient implementations in the core is because of dependencies. Ideally core should just be a bunch of interfaces and then there would be a client assembly that would put a bunch of those interfaces together to do make up common client behavior. Right now we don't have that split, and it's why the "Official NuGet Feed" is defined in both the VS assembly and in the command line.

Jan 5, 2011 at 9:59 PM
davidebbo wrote:

Sure, you can try using a new exception.  But make sure you pass the original one as the inner exception, to make sure that nothing is lost.

Other comment: weren't you going to pass the key in the ctor instead of into each API?

I was going to pass the Key in the CTOR but there is one API that doesn't use it (RatePackage) and I figured there might be more in the future and if you just wanted to call those methods it was pretty crappy. Another benefit of passing the key in to each method is that the GalleryServer object becomes package/user agnostic.