1172899 - Queue: createArtifact - Fixed structure req/res in APIs (ie avoid oneOf in json schema)

Status: RESOLVED WONTFIX

(Taskcluster :: Services, defect)

Description

[Pete Moore [:pmoore][:pete]]

For taskcluster clients that provide static type structures to model json request/response payloads, json schema conformity of requests/responses is guaranteed at compile-time to consumers of the library.

For example, when using the go taskcluster client library to access an API endpoint, a misspelling of a required property in a json request schema will result in a compile-time error to the consuming project.

However, when compiling static types to represent json schema objects, json subschemas with "oneOf" attributes cannot easily be mapped to custom types, since the properties of such a time are not frozen.

With the addition of considerable complexity, in the go client this might be possible using json.RawMessage by generating code similar to the example in:
http://golang.org/pkg/encoding/json/#example_RawMessage. However, even then, it would require client code to manage type switching at runtime, building of interfaces to abstract results, and an overall loss in the ability to infer structural errors at compile time.

An example of this is the http://schemas.taskcluster.net/queue/v1/post-artifact-request.json schema used by http://docs.taskcluster.net/queue/api-docs/#createArtifact. Since the request can take several different forms (depending on the artifact type), it means the go client cannot (easily) ensure at compile time that the correct parameters have been passed in. Instead library consumers (such as generic worker) need to generate json.RawMessage at runtime with a []byte storing the json representation of the input, which means a) the benefits of compile-time type management are not available, b) consumer code is more complex and fragile, and more likely to contain bugs.

In this case, the easiest solution to this would be to have several createArtifact methods, e.g.: createS3Artifact, createAzureArtifact, createRedirectArtifact, createErrorArtifact. Each would have its own request/response schema, not containing "oneOf" attributes, and then custom go request/response types would be automatically generated for each.

The createArtifact method could either be replaced by these artifact-type-specific methods, or the new methods could live alongside the legacy method.

I'm not sure if this is the only occurrence or if there are also other API methods with oneOf. If we agree we'd like to make this change, I will also scan for other offending methods.

Comment 1

[Pete Moore [:pmoore][:pete]]

Let me know what you think...

Comment 2

[Jonas Finnemann Jensen (:jonasfj)]

I would kind of love to have this as separate methods.
However, I like that the URL pattern is:
  POST /task/<taskId>/runs/<runId>/artifacts/<name>

hmm... Maybe there is something smart we can do here...
We've discussed the addition of new API methods in bug 1155645, to ensure that uploads follow
a simple three step process:
 1) defineArtifact (get signed url)
 2) upload to signed url
 3) publishArtifact (make it visible on queue)

Maybe it could be:
  createS3Artifact         as      POST /task/<taskId>/runs/<runId>/artifact:s3/<name>
  createAzureArtifact      as      POST /task/<taskId>/runs/<runId>/artifact:azure/<name>
  createReferenceArtifact  as      POST /task/<taskId>/runs/<runId>/artifact:reference/<name>
  createErrorArtifact      as      POST /task/<taskId>/runs/<runId>/artifact:error/<name>
Then followed by:
  publishArtifact          as      PUT  /task/<taskId>/runs/<runId>/artifacts/<name>
Then `publishArtifact` would work regardless of artifact type. Although it would be unnecessary for
to call `publishArtifact` for reference and error artifacts as they are created atomically.

Hmm, I have mixed feeling about this. I think maying splitting them is the sane thing to do here.
I'm playing with refactoring artifacts implementation a bit, but any ideas/suggestions/concerns are
most welcome. It's been a while since I justified the current implementation.

Comment 3

[Pete Moore [:pmoore][:pete]]

I'll add this to the Berlin agenda.

I see recent updates to:

* http://schemas.taskcluster.net/queue/v1/post-artifact-request.json
* http://schemas.taskcluster.net/queue/v1/post-artifact-response.json

so if we are revising this at the moment, it might be good to incorporate changes for this bug too...

CC'ing Greg as this will affect docker-worker also.

Pete

Comment 4

[Jonas Finnemann Jensen (:jonasfj)]

Let's just not fix this...

Comment 5

[Jonas Finnemann Jensen (:jonasfj)]

jhford is refactoring queue artifact APIs and this won't happen..

We can likely fix the code gen in golang instead..