osu-capstone-cs72 / cs-applied-plan-portal

A portal that streamlines the planning process for OSU CS Applied students and advisors
https://applied-plan-portal.herokuapp.com
MIT License
2 stars 5 forks source link

Standardize JSON responses #33

Closed JacksGo closed 4 years ago

JacksGo commented 4 years ago

The API server should return a bona fide JSON object in all responses, rather than just returning an array or a value. Extension of #29.

Possible examples:

{
  "success": true,
  "data": [
     {
       "name": "CS271"
     }
   ]
}
{
  "success": false,
  "error": "haha, oops"
}
philectron commented 4 years ago

Just to clarify from our talk, we all have agreed on this standardized JSON response, as follows:

  1. The response object shall always be a JSON object.
  2. The object shall always contain success field indicating whether the request was successfully handled or not.
  3. If the success field is true, there shall be another field named data that shall be an array containing result object (or objects). Even if there is a single result object every time, the data field shall always be an array.
  4. If the success field is false, there shall be another field named error that specifies an error message string.

This allows us to maintain the responses a lot more easily. If we need additional fields in the future, simply add them to the root object.

silverware13 commented 4 years ago

@philectron I want to add on to the standardization rule if you agree. I thought at first that data made sense for the return for all array responses, but as I have been updating the plan endpoints I realized it should be specific based on the request.

For example here is the newest response for a "get plan by id" request: { "planId": 310, "status": 0, "planName": "Han's cool plan", "studentId": 5, "created": "2020-01-02T11:35:42.000Z", "lastUpdated": "2020-02-16T19:54:53.000Z", "courses": [ { ...data about the first course... }, { ...data about the second course... }, ... ] }

In this case instead of just having the array as "data" we use a more specific named pair "courses". For a list of plans returned by user id instead of "data" you would call your array "plans".

Let me know if this sounds okay to you.

JacksGo commented 4 years ago

This is a late reply, but my initial instinct would be to use the following structure for that request:

{
  "success": true,
  "data": {
    "id": 310,
    "name": "Han's Cool Plan",
    "status": 0,
    "author": 5,
    "created": 1577964942,
    "updated": 1581882893,
    "courses": [
      {
        "id": 32512,
        "name": "CS 462"
      },
      {
        "id": 37133,
        "name": "CS 492"
      }
    ]
  }
}
JacksGo commented 4 years ago

To elaborate, the idea behind standardizing our responses in this way is to ensure that the structure of certain parts of our responses (e.g., the top level keys) is consistent and immutable. From there, we define a specific key ("data") under which dynamic content is stored. Done correctly, this could theoretically allow us to hot-swap endpoints in the code without changing the parsing logic and still be able to retrieve data. This makes things more extensible (imo), easier for us, and easier for (hypothetical) people to use our API endpoints. I'd argue it's much cleaner to contain returned data under a single top-level key. For a contrived example, with this structure, we could use a "success" key in our data, where previously we couldn't.

philectron commented 4 years ago

I guess an example for error responses will be like this?

{
  "success": false,
  "error": {
    "code": 400,
    "message": "Plan name must be at least 5-character long."
  }
}

I really like this standardized JSON response, by the way.

silverware13 commented 4 years ago

I have some objections still. My biggest issue is that the response we get back already does all this wrapping for us, so it seems redundant to wrap it again.

Example of a response:

type: "cors"
url: "http://localhost:5000/plan/375?accessToken=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOjgyNzU3NTc5NTI3LCJyb2xlIjowLCJpYXQiOjE1ODI4NDk1MjksImV4cCI6MTU4MjkzNTkyOX0.0OaHpRjC3g1du3L5IOKUKKeWVgYf_iosU2SU_njNFG0"
redirected: false
status: 200
ok: true
statusText: "OK"
headers: Headers {}
body: (...)
bodyUsed: true

The success field does not make sense to me as that is already provided by the ok field. Likewise wrapping everything else in data doesn't make sense as that is what the body does for us. Having everything wrapped also makes the data more awkward to work with on the client side.

No wrappers:

  if (response.ok) {
    obj = await response.json();
    planName = obj.planName;
    planId = obj.planId;
    courses = obj.courses;
  }

With wrappers:

  if (response.ok) {
    obj = await response.json();
    if (obj.success) {
      planName = obj.data.planName;
      planId = obj.data.planId;
      courses = obj.data.courses;
    }
  }

Lastly it goes against the conventions of other popular REST APIs:

The Open Weather API: https://openweathermap.org/current

{
  "coord": { "lon": 139,"lat": 35},
  "weather": [
    {
      "id": 800,
      "main": "Clear",
      "description": "clear sky",
      "icon": "01n"
    }
  ],
  "base": "stations",
  "main": {
    "temp": 281.52,
    "feels_like": 278.99,
    "temp_min": 280.15,
    "temp_max": 283.71,
    "pressure": 1016,
    "humidity": 93
  },
  "wind": {
    "speed": 0.47,
    "deg": 107.538
  },
  "clouds": {
    "all": 2
  },
  "dt": 1560350192,
  "sys": {
    "type": 3,
    "id": 2019346,
    "message": 0.0065,
    "country": "JP",
    "sunrise": 1560281377,
    "sunset": 1560333478
  },
  "timezone": 32400,
  "id": 1851632,
  "name": "Shuzenji",
  "cod": 200
}

GitHub API: https://developer.github.com/v3/

{
  "login": "octocat",
  "id": 1,
  "node_id": "MDQ6VXNlcjE=",
  "avatar_url": "https://github.com/images/error/octocat_happy.gif",
  "gravatar_id": "",
  "url": "https://api.github.com/users/octocat",
  "html_url": "https://github.com/octocat",
  "followers_url": "https://api.github.com/users/octocat/followers",
  "following_url": "https://api.github.com/users/octocat/following{/other_user}",
  "gists_url": "https://api.github.com/users/octocat/gists{/gist_id}",
  "starred_url": "https://api.github.com/users/octocat/starred{/owner}{/repo}",
  "subscriptions_url": "https://api.github.com/users/octocat/subscriptions",
  "organizations_url": "https://api.github.com/users/octocat/orgs",
  "repos_url": "https://api.github.com/users/octocat/repos",
  "events_url": "https://api.github.com/users/octocat/events{/privacy}",
  "received_events_url": "https://api.github.com/users/octocat/received_events",
  "type": "User",
  "site_admin": false,
  "name": "monalisa octocat",
  "company": "GitHub",
  "blog": "https://github.com/blog",
  "location": "San Francisco",
  "email": "octocat@github.com",
  "hireable": false,
  "bio": "There once was...",
  "public_repos": 2,
  "public_gists": 1,
  "followers": 20,
  "following": 0,
  "created_at": "2008-01-14T04:33:35Z",
  "updated_at": "2008-01-14T04:33:35Z"
}

Microsoft Vision API: https://azure.microsoft.com/en-us/services/cognitive-services/computer-vision/

{ 
  "tags": [ "train", "platform", "station", "building", "indoor", "subway", "track", "walking", "waiting", 
    "pulling", "board", "people", "man", "luggage", "standing", "holding", "large", "woman", "yellow", 
    "suitcase" ], 
  "captions": [ { "text": "people waiting at a train station", "confidence": 0.8330992 } ]
 }
philectron commented 4 years ago

@silverware13 I see your point. I forgot that the React server receives more than just the JSON response from the API server. Now that I've thought about it, I'm going with keeping the API responses as simple as possible.

Still, to me, a JSON response must have curly brackets { } at the root level. So, if an API endpoint reponds with a single array, that array must be wrapped into the brackets, e.g.

{
  "items": [
    {
      "foo": "bar"
    },
    {
      "foo": "baz"
    },
    {
      "foo": "bruh"
    }
  ]
}

Other than that, even though I like Jackson's standardized format, I think it is redundant, as the responses the React server receives already have some helpful overheads to it.

Note that the name of the array should not be hardcoded to items but instead should be relevant to the API, e.g. courses or plans

silverware13 commented 4 years ago

I agree with your example of a JSON object. Arrays must be expressed as named fields.

JacksGo commented 4 years ago

@silverware13 I'm going to respectfully disagree. Rather than starting a back and forth providing examples of real-world APIs that either follow or buck the standard, I'll note that my proposal is consistent with the JSON:API standard, a widely used, community driven standard for API consistency. I fundamentally disagree with the notion that the fetch API makes encapsulation unnecessary. What if we switch to something other than fetch?

I also disagree that following my proposal makes manipulating the client side more difficult; on the contrary, I'd argue it makes the process simpler. For instance, you can simply call if (res.success) { ... }, instead of checking the response against a variety of possible status codes. If the success field is guaranteed to be in the response every time, there's no risk of an error being thrown if we try to access it on a non-compliant response.

In my view, it ultimately boils down to the same reason why you would include a count attribute along with an array, instead of just trusting users to compute the length themselves. It's shorter, bytes-wise, to omit it; it's arguably more lines of code to parse it -- but at the end of the day, it fails to simplify and standardize a very common use case.

JacksGo commented 4 years ago

I'm not the subject matter expert on the back-end, however, so I'll defer to your and Phi's judgement.

philectron commented 4 years ago

Honestly, at this point, I don't know. You both make very good points on why we should or shouldn't have overheads in the API responses, and that makes me very indecisive right now.

I don't think the array-count analogy applies here, though. In a hypothetical language, I think that the count attribute exists to make the operation that retrieves the size more efficient (O(1) instead of O(n), traded off with additional, say, 4 bytes of memory to hold an integer count). The problem here is just the consistency of the way we format our API responses. But I digress.

With the current format, the API responses are the data field itself--nothing more, nothing less. To check whether the response was okay or not, we only need to use response.ok. Plus, we always know the status of the response by response.status. What I like about this format is its simplicity. This is the part where I'd say "let abstraction happen naturally."

With Jackson's proposed format, we can dictate the success/failure, the status, the data, and any additional information that we may add in the future. What I like about this format is that we don't have to depend on anything besides the response itself to determine the success or failure of the operation. This format, to me, is more consistent and flexible.

That's just my thought on each of the formats. I don't have a strong preference for either of those, but I'm a little bit inclined towards the new format. I'll take a few days to think about this issue.

silverware13 commented 4 years ago

@JacksGo Some additional points I would make:

You give your example as being consistent with the JSON:API standard. My example is also consistent with the JSON:API standard.

From the homepage of the JSON:API spec: https://jsonapi.org/

{
  "links": {
    "self": "http://example.com/articles",
    "next": "http://example.com/articles?page[offset]=2",
    "last": "http://example.com/articles?page[offset]=10"
  },
  "data": [{
    "type": "articles",
    "id": "1",
    "attributes": {
      "title": "JSON:API paints my bikeshed!"
    },
    "relationships": {
      "author": {
        "links": {
          "self": "http://example.com/articles/1/relationships/author",
          "related": "http://example.com/articles/1/author"
        },
        "data": { "type": "people", "id": "9" }
      },
      "comments": {
        "links": {
          "self": "http://example.com/articles/1/relationships/comments",
          "related": "http://example.com/articles/1/comments"
        },
        "data": [
          { "type": "comments", "id": "5" },
          { "type": "comments", "id": "12" }
        ]
      }
    },
    "links": {
      "self": "http://example.com/articles/1"
    }
  }],
  "included": [{
    "type": "people",
    "id": "9",
    "attributes": {
      "firstName": "Dan",
      "lastName": "Gebhardt",
      "twitter": "dgeb"
    },
    "links": {
      "self": "http://example.com/people/9"
    }
  }, {
    "type": "comments",
    "id": "5",
    "attributes": {
      "body": "First!"
    },
    "relationships": {
      "author": {
        "data": { "type": "people", "id": "2" }
      }
    },
    "links": {
      "self": "http://example.com/comments/5"
    }
  }, {
    "type": "comments",
    "id": "12",
    "attributes": {
      "body": "I like XML better"
    },
    "relationships": {
      "author": {
        "data": { "type": "people", "id": "9" }
      }
    },
    "links": {
      "self": "http://example.com/comments/12"
    }
  }]
}

The example they give does not use a success field and while they use a data field, everything isn't encapsulated in it, but rather into many fields. So I think it would be fair to say that neither your or my example violates this standard. Edit: I read the link a bit more thoroughly and in their example they divide all of the responses into included, data, link, and error. So I guess neither of ours 100% adheres to that (success is not wrapped inside data, my responses aren't wrapped into multiple categories, etc). Though I personally don't feel that we return enough data to require having that many different categories of data in our responses.

As for accommodating methods other than fetch, I would say that whoever is designing the front-end will always have to be aware of the API interface they are using. I assume that every modern interface has a way to check the status, success/ok, and the body of the response.

The last reason I don't really agree with including a success field is that it isn't safe to check. For example, what if I make a request to an API server and the server has an error that prevents it from sending the body. There will be no success field to check. However we could still check the ok field in the response safely.

philectron commented 4 years ago

This issue has been resolved in the 2020/03/03 meeting, so I'm closing it.

We are using the HTTP response to determine success and failure. No changes needed.