API consistency: OK, response only has "data", remove okResponseGsonObject

[pdurbin]

Whenever the native API returns an OK response, it returns a JSON object
with the key "data" and lots of detail within. "data" is the only key
returned so I'm moving "message" to be within the "data" object to be
consistent with the rest of the native API.

I'm also removing the okResponseGsonObject method from the
AbstractApiBean for consistency. We don't want it to be cluttered with
Gson, Jackson or other methods.

[raprasad]

@scolapasta @pdurbin, question API consistency for the message attribute

Currently API response for errors:

• message is a top level key
  • Example:

{
    "status": "ERROR",
    "message": "This file has a duplicate already in the dataset: dataverseproject-1.png"
}

Successful API responses

• Shouldn't message also be a top level key, not within data:

{
    "status": "OK", 
    "data": {
       "message": "File successfully added!",
        "files": [
            {
                "dataFile": {
                    "originalFormatLabel": "UNKNOWN", 
                    "contentType": "text/csv", 
                    "description": "Blue skies!", 
                    "rootDataFileId": -1, 
                    "checksum": {
                        "type": "MD5", 
                        "value": "f8a112f69bd4419466db2efca0df7e55"
                    }, 
                    "tags": [], 
                    "filename": "add_0001-15.csv", 
                    "filesize": 105, 
                    "md5": "f8a112f69bd4419466db2efca0df7e55", 
                    "id": 417, 
                    "categories": [
                        "Data", 
                        "Glue", 
                        "Foo", 
                        "Blue", 
                        "Zoo"
                    ], 
                    "storageIdentifier": "1580289e73a-09fd5635e516"
                }, 
                "datasetVersionId": 191, 
                "version": 1, 
                "description": "Blue skies!", 
                "label": "add_0001-15.csv"
            }
        ]
    }
}

[pdurbin]

@raprasad that's right. "message" is returned for errors. Here are the rules:

• On error, return "message".
• On success, return "data".

I didn't write the code. @michbarsinai did. I'm just trying to keep the API consistent.

[raprasad]

@pdurbin : I kept the message convention. Note, moved any JSON generating code out of AddReplaceFileHelper and into JSON printer. e.g. passing JSON Printer a list of DataFiles

[raprasad]

Closing. Covered in 1612 branch.

[scolapasta]

I do like the idea of message not being in data. So maybe we can ping
@michbarsinai, like you said and see if there's any reason not to do it
(consistently) that way.

On Wed, Oct 26, 2016 at 4:18 PM, Raman Prasad notifications@github.com
wrote:

@pdurbin
https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_pdurbin&d=CwMCaQ&c=WO-RGvefibhHBZq3fL85hQ&r=xgAtlRJhZJjMyKX1J660N0IQRNfpw7XyvKRWo-rQjyo&m=cfkFK1IqGCaokYzo1vS5kUuL3teeQvQ7ZD60QXj_Sfs&s=adR-yatbkj4hprepezpjh-PwrMugp6lSjSpQ5cPvvhg&e=
: I kept the message convention. Note, moved any JSON generating code out
of AddReplaceFileHelper and into JSON printer. e.g. passing JSON Printer a
list of DataFiles

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_IQSS_dataverse_pull_3433-23issuecomment-2D256464693&d=CwMCaQ&c=WO-RGvefibhHBZq3fL85hQ&r=xgAtlRJhZJjMyKX1J660N0IQRNfpw7XyvKRWo-rQjyo&m=cfkFK1IqGCaokYzo1vS5kUuL3teeQvQ7ZD60QXj_Sfs&s=ngWPa8-hPKXYX6B7fCol97GzD3hJ6pmIupHur6zIdG0&e=,
or mute the thread
https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AEEhBCN8iDqKqkKELvP2i-5FGaHgwigjbBks5q37WfgaJpZM4Khl02&d=CwMCaQ&c=WO-RGvefibhHBZq3fL85hQ&r=xgAtlRJhZJjMyKX1J660N0IQRNfpw7XyvKRWo-rQjyo&m=cfkFK1IqGCaokYzo1vS5kUuL3teeQvQ7ZD60QXj_Sfs&s=BXyXoyjItcMNhi9o_klCmIo25h61Jev50rLVQKYEXuE&e=
.

[pdurbin]

@sbarbosadataverse @raprasad I slept on it and it and while I still want consistency, I think we should consider creating a GitHub issue called "Have consistent location of success messages in the API". Think about building a GUI in React of Angluar on top of the Dataverse API. Users are clicking things and you want to display success messages to them. This would be the equivalent of our JsfHelper.addSuccessMessage pattern. Right now everything is just thrown into "data" and you don't see stuff like "Dataset created." All you get back is a dataset id:

{
    "status": "OK",
    "data": {
        "id": 37
    }
}

It would be hard to build a UI in React or Angular from the JSON output above for creating a dataset. Yes, let's talk to @michbarsinai and get his thoughts.

[michbarsinai]

Hi Guys. Sorry to jump so late on this issue. I think the point the discussion is at (as far as I understand it) is correct: on success, we return the payload in data. On failure, we return a human readable reason in message. The status field tells the client which field to access.

The differentiation between data and message is very intentional. In particular, if there was a reason because of which Dataverse was not able to retrieve the requested data, there is no data in the response, and thus it should contain no data field. Putting a textual message in data would make its semantics ambiguous, which is never good. This is especially important in cases the API endpoint returns a string.

This design where a value at a known field tells the client how to access the rest of the object is not new or unique to us. It's the main reason C has union types. Other examples include Either/Or monads, PlayFramework's JsSuccess and JsFailure, both extending JsResult(https://www.playframework.com/documentation/2.5.x/api/scala/index.html#play.api.libs.json.JsResult), manifest files, WEB-INF, etc. etc.

Note that the HTTP protocol gives us another place to put the result - the response code. This is why we have response methods like created() (https://github.com/IQSS/dataverse/blob/develop/src/main/java/edu/harvard/iq/dataverse/api/AbstractApiBean.java#L489) and accepted(), not just ok.

This design breaks the DIY principle in that the status field somewhat repeats the data in the HTTP response code, in a less detailed form. The idea behind this was to be curl | jq friendly - piping these programs does not allow curl to print out the response code, which is not in JSON format (and thus breaks jq). Additionally, it's more friendly to beginning programmers that find dealing with the payload a challenge, and so also having to deal with the HTTP response code (what's that?!) is becoming overwhelming. Think technically inclined post-docs that know some R code and want to hack something fast and with little effort.

Lastly, I don't see any problem with writing UI based on the API as they are now, regardless of the technology being used. HTTP clients maintain state between request and response, and so when the client gets an answer it should know what the question was.

[raprasad]

@michbarsinai : so you agree(?) that we can move message outside of data on successful calls. e.g. we would like to include a success message for the general assistance--especially to beginning programmers.:

{
    "status": "OK (or created)",
    "message": "File successfully added!",
    "data": {
      "file data" : "lots of info"
   }
}

[michbarsinai]

Is message in data now? it should be moved out. It's a semantic error to keep it in (again, especially in cases where the payload contains a legitimate message field).

As for user-friendly messages, it's API. It should not be UI-friednly on the successful path, since you're making an assumption on the UI itself. For one thing, the below message can't be localized. Or an app might decide to show a green checkmark near the file icon instead of a message. I think we're better off not having messages when we have payload.

Ideally we'd also do the errors in a UI-independent way, but it's much harder. And the HTTP response is indeed UI independent.

On 27 Oct 2016, at 21:01, Raman Prasad notifications@github.com wrote:

@michbarsinai https://github.com/michbarsinai : so you agree(?) that we can move message outside of data on successful calls. e.g. we would like to include a success message for the general assistance--especially to beginning programmers.:

{
"status": "OK (or created)",
"message": "File successfully added!",
"data": {
"file data" : "lots of info"
}
}
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #3433 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AB2UJE7M0-I7Pj8pPKn7qcnoX0LyXwQUks5q4OcHgaJpZM4Khl02.

[raprasad]

Is message *in* data now? it should be moved out.

Good. Agreed.

[raprasad]

(whew) yes, it was in some of the original api code this way:

  protected Response ok( String msg ) {
        return Response.ok().entity(Json.createObjectBuilder()
            .add("status", STATUS_OK)
            .add("data", Json.createObjectBuilder().add("message",msg)).build() )
            .type(MediaType.APPLICATION_JSON)
            .build();
    }

[pdurbin]

@michbarsinai @raprasad it sounds like you're talking about changing the 49 occurrences of "message" being nested with "data" in the "OK" case. Is it time to version the API? Backwards compatibility is important. Maybe I'm misunderstanding you. I'm glad we're doing pull requests and code review these days. 😄

[pdurbin]

@michbarsinai @scolapasta please code review this commit by @raprasad to make sure it's what we want before it gets merged: 0540ce0

[michbarsinai]

It's a simple rule, really. If the message is an OK one (2XX) payload goes in the data object. The payload may be a message, if it makes sense in the context, or it may be something more complex. The cases where we use ok(String message) are simple cases where the message could be ignored. I assume most of them could be moved to a more detailed HTTP response (201-204)*. data is always an object, not a String, which is why the message field is there when we want to return a message string.

In the general case, API clients need not rely on textual messages from the server to communicate a request result to the user.

I see where this pull request comes from, but IMHO it should not be merged.
You can look at the status field as the internal class field in a Java object. OK results have no top-level message field. ERROR results have no data field, but they do have a top-level message field. Here's the pseudo-code class hierarchy:

abstract class DataverseResponse {
   int httpCode;
}

class OK extends DataverseResponse {
   Map<KEY,Object> data;
}

class Error extends DataverseResposne {
   String message;
}

As for versioning the API, I think we have more urgent things to do now. If we were to do it, adding details to error messages might be a nice thing to add. We currently use the message field there since software does not have to be perfect, but it has to be delivered :-).

• https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html