[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4716: API Guidelines

[GitBox <gi...@apache.org>]

ocket8888 commented on a change in pull request #4716:
URL: https://github.com/apache/trafficcontrol/pull/4716#discussion_r430715956



##########
File path: docs/source/development/api_guidelines.rst
##########
@@ -0,0 +1,268 @@
+..
+..
+.. Licensed under the Apache License, Version 2.0 (the "License");
+.. you may not use this file except in compliance with the License.
+.. You may obtain a copy of the License at
+..
+..     http://www.apache.org/licenses/LICENSE-2.0
+..
+.. Unless required by applicable law or agreed to in writing, software
+.. distributed under the License is distributed on an "AS IS" BASIS,
+.. WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+.. See the License for the specific language governing permissions and
+.. limitations under the License.
+..
+
+.. _api-guidelines:
+
+**************
+API Guidelines
+**************
+This section lays out guidelines to be considered by API endpoint authors. What follows are not *strictly* hard-and-fast rules, but there should be a very convincing argument accompanying endpoints that do not follow them.
+
+Legacy API versions don't, in general, adhere to these principles. This section is not meant as a proposal for altering them to be compliant, but merely a set of guidelines for future work.
+
+Response Bodies
+===============
+All valid API responses will be in the form of some serialized object. The main data that represents the result of the client's request MUST appear in the ``response`` property of that object. If a warning, error message, success message, or informational message is to be issued by the server, then they MUST appear in the ``alerts`` property of the response. Some endpoints may return ancillary statistics such as the total number of objects when pagination occurs, which should be placed in the ``summary`` property of the response.
+
+Response
+--------
+The ``response`` property of a serialized response object MUST only contain object representations as requested by the client. In particular, it MUST NOT contain admonitions, success messages, informative messages, or statistic summaries beyond the scope requested by the client.
+
+Equally unacceptable API responses are shown in :ref:`success-as-response` and :ref:`extra-field`.
+
+.. _success-as-response:
+.. code-block:: json
+	:caption: Success Message as Response Object
+
+	{
+		"response": "Thing was successfully created."
+	}
+
+.. _extra-field:
+.. code-block:: json
+	:caption: Illegal Top-Level Property
+
+	{
+		"response": {"foo": "bar"},
+		"someOtherField": {"someOtherObject"}
+	}
+
+When requests are serviced by Traffic Ops that pass data asking that the returned object list be filtered, the response property MUST be a filtered array of those objects (assuming the request may be successfully serviced). This is true even if filtering is being done according to a uniquely identifying property - e.g. a numeric ID. The ``response`` field of an object returned in response to a request to create, update, or delete one or more resources may be either a single object representation or an array thereof according to the number of objects created, updated, or deleted. However, if a handler is *capable* of creating, updating, or deleting more than one object at a time, the ``response`` field SHOULD consistently be represented as an array - even if its length would only be 1.
+
+The proper value of an empty collection is an empty collection. If a Foo can have zero or more Bars, then the representation of a Foo with no Bars MUST be an empty array/list/set, and in particular MUST NOT be either missing from the representation or represented as the "Null" value of the representation format. That is, if Foos have no other property than their Bars, then a Foo with no Bars may be represented in JSON encoding as ``{"bars":[]}``, but not as ``{"bars":null}`` or ``{}``. Similarly, an empty string field is properly represented as an empty string - e.g. ``{"bar":""}`` not ``{"bar":null}`` or ``{}`` - and the "zero-value" of numbers is zero itself - e.g. ``{"bar":0}`` not ``{"bar":null}`` or ``{}``. Note that "null" values *are allowed* when appropriate, but "null" values represent the *absence* of a value rather than the "zero-value" of a property. If a property is *missing* from an object representation it indicates the absence of that property, and because of that there must be a *very convincing* argument if and when that is the case.
+
+As a special case, endpoints that report statistics including minimums, maximums and arithmetic means of data sets MUST use the property names ``min``, ``max``, and ``mean``, respectively, to express those concepts. These SHOULD be properties of ``response`` directly whenever that makes sense.
+
+Alerts
+------
+Alerts should be presented as an array containing objects which each conform to the object definition laid out by :atc-godoc:`the ATC library's Alert structure <lib/go-tc#Alert>`. The allowable ``level``\ s of an Alert are:
+
+- ``error`` - This level MUST be used to indicate conditions that caused a request to fail. Because of this, this level MUST NOT appear in the ``alerts`` array of responses with any HTTP response code less than 400 (except when used for asynchronous tasks as discussed in `202 Accepted`_). Details of server workings and/or failing components MUST NOT be exposed in this message, which should otherwise be as descriptive as possible.
+- ``info`` - This level SHOULD be used to convey supplementary information to a user that is not directly the result of their request. This SHOULD NOT carry information indicating whether or not the request succeeded and why/why not, as that is best left to the ``error`` and ``success`` levels.
+- ``success`` - This level MUST be used to convey success messages to the client. In general, it is expected that the message will be directly displayed to the user by the client, and therefore can be used to add human-friendly details about a request beyond the response payload. This level MUST NOT appear in the ``alerts`` array of responses with an HTTP response code that is not between 200 and 399 (inclusive).
+- ``warning`` - This level is used to warn clients of potentially dangerous conditions when said conditions have not caused a request to fail. The best example of this is deprecation warnings, which should appear on all API routes that have been deprecated.
+
+Summary
+-------
+The ``summary`` object is used to provide summary statistics about object collections. In general the use of ``summary`` is left to be defined by API endpoints (subject to some restrictions). However, its use is not appropriate in cases where the user is specifically requesting summary statistics, but should rather be used to provide supporting information - pre-calculated - about a set of objects or data that the client *has* requested.
+
+Endpoints MUST use the following, reserved properties of ``summary`` for their described purposes (when use of ``summary`` is appropriate) rather than defining new ``summary`` or ``response`` properties to suit the same purpose:
+
+- ``count`` - Count contains an unsigned integer that defines the total number of results that could possibly be returned given the non-pagination query parameters supplied by the client.

Review comment:
       No, that's in the "Response" object section

##########
File path: docs/source/development/api_guidelines.rst
##########
@@ -0,0 +1,268 @@
+..
+..
+.. Licensed under the Apache License, Version 2.0 (the "License");
+.. you may not use this file except in compliance with the License.
+.. You may obtain a copy of the License at
+..
+..     http://www.apache.org/licenses/LICENSE-2.0
+..
+.. Unless required by applicable law or agreed to in writing, software
+.. distributed under the License is distributed on an "AS IS" BASIS,
+.. WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+.. See the License for the specific language governing permissions and
+.. limitations under the License.
+..
+
+.. _api-guidelines:
+
+**************
+API Guidelines
+**************
+This section lays out guidelines to be considered by API endpoint authors. What follows are not *strictly* hard-and-fast rules, but there should be a very convincing argument accompanying endpoints that do not follow them.
+
+Legacy API versions don't, in general, adhere to these principles. This section is not meant as a proposal for altering them to be compliant, but merely a set of guidelines for future work.
+
+Response Bodies
+===============
+All valid API responses will be in the form of some serialized object. The main data that represents the result of the client's request MUST appear in the ``response`` property of that object. If a warning, error message, success message, or informational message is to be issued by the server, then they MUST appear in the ``alerts`` property of the response. Some endpoints may return ancillary statistics such as the total number of objects when pagination occurs, which should be placed in the ``summary`` property of the response.
+
+Response
+--------
+The ``response`` property of a serialized response object MUST only contain object representations as requested by the client. In particular, it MUST NOT contain admonitions, success messages, informative messages, or statistic summaries beyond the scope requested by the client.
+
+Equally unacceptable API responses are shown in :ref:`success-as-response` and :ref:`extra-field`.
+
+.. _success-as-response:
+.. code-block:: json
+	:caption: Success Message as Response Object
+
+	{
+		"response": "Thing was successfully created."
+	}
+
+.. _extra-field:
+.. code-block:: json
+	:caption: Illegal Top-Level Property
+
+	{
+		"response": {"foo": "bar"},
+		"someOtherField": {"someOtherObject"}
+	}
+
+When requests are serviced by Traffic Ops that pass data asking that the returned object list be filtered, the response property MUST be a filtered array of those objects (assuming the request may be successfully serviced). This is true even if filtering is being done according to a uniquely identifying property - e.g. a numeric ID. The ``response`` field of an object returned in response to a request to create, update, or delete one or more resources may be either a single object representation or an array thereof according to the number of objects created, updated, or deleted. However, if a handler is *capable* of creating, updating, or deleting more than one object at a time, the ``response`` field SHOULD consistently be represented as an array - even if its length would only be 1.
+
+The proper value of an empty collection is an empty collection. If a Foo can have zero or more Bars, then the representation of a Foo with no Bars MUST be an empty array/list/set, and in particular MUST NOT be either missing from the representation or represented as the "Null" value of the representation format. That is, if Foos have no other property than their Bars, then a Foo with no Bars may be represented in JSON encoding as ``{"bars":[]}``, but not as ``{"bars":null}`` or ``{}``. Similarly, an empty string field is properly represented as an empty string - e.g. ``{"bar":""}`` not ``{"bar":null}`` or ``{}`` - and the "zero-value" of numbers is zero itself - e.g. ``{"bar":0}`` not ``{"bar":null}`` or ``{}``. Note that "null" values *are allowed* when appropriate, but "null" values represent the *absence* of a value rather than the "zero-value" of a property. If a property is *missing* from an object representation it indicates the absence of that property, and because of that there must be a *very convincing* argument if and when that is the case.
+
+As a special case, endpoints that report statistics including minimums, maximums and arithmetic means of data sets MUST use the property names ``min``, ``max``, and ``mean``, respectively, to express those concepts. These SHOULD be properties of ``response`` directly whenever that makes sense.
+
+Alerts
+------
+Alerts should be presented as an array containing objects which each conform to the object definition laid out by :atc-godoc:`the ATC library's Alert structure <lib/go-tc#Alert>`. The allowable ``level``\ s of an Alert are:
+
+- ``error`` - This level MUST be used to indicate conditions that caused a request to fail. Because of this, this level MUST NOT appear in the ``alerts`` array of responses with any HTTP response code less than 400 (except when used for asynchronous tasks as discussed in `202 Accepted`_). Details of server workings and/or failing components MUST NOT be exposed in this message, which should otherwise be as descriptive as possible.
+- ``info`` - This level SHOULD be used to convey supplementary information to a user that is not directly the result of their request. This SHOULD NOT carry information indicating whether or not the request succeeded and why/why not, as that is best left to the ``error`` and ``success`` levels.
+- ``success`` - This level MUST be used to convey success messages to the client. In general, it is expected that the message will be directly displayed to the user by the client, and therefore can be used to add human-friendly details about a request beyond the response payload. This level MUST NOT appear in the ``alerts`` array of responses with an HTTP response code that is not between 200 and 399 (inclusive).
+- ``warning`` - This level is used to warn clients of potentially dangerous conditions when said conditions have not caused a request to fail. The best example of this is deprecation warnings, which should appear on all API routes that have been deprecated.
+
+Summary
+-------
+The ``summary`` object is used to provide summary statistics about object collections. In general the use of ``summary`` is left to be defined by API endpoints (subject to some restrictions). However, its use is not appropriate in cases where the user is specifically requesting summary statistics, but should rather be used to provide supporting information - pre-calculated - about a set of objects or data that the client *has* requested.
+
+Endpoints MUST use the following, reserved properties of ``summary`` for their described purposes (when use of ``summary`` is appropriate) rather than defining new ``summary`` or ``response`` properties to suit the same purpose:
+
+- ``count`` - Count contains an unsigned integer that defines the total number of results that could possibly be returned given the non-pagination query parameters supplied by the client.
+
+HTTP Request Methods
+====================
+:RFC:`7231#section-4` defines the semantics of HTTP/1.1 request methods. Authors should conform to that set of standards whenever possible, but for convenience the methods recognized by Traffic Ops and their meanings in that context are herein defined.
+
+GET
+---
+HTTP GET requests are issued by clients who want some data in response. In the context of Traffic Ops, this generally means a serialized representation of some object. GET requests MUST NOT alter the state of the server. An example of an API endpoint created in API version 1 that violates this restriction is :samp:`cdns/name/{name}/dnsseckeys/delete`.
+
+This is the standard method to be used by all read-only operations, and as such handlers for this method should generally be accessible to users with the "read-only" :term:`Role`.
+
+All endpoints dealing with the manipulation or fetching representations of "Traffic Control Objects" MUST support this method.
+
+POST
+----
+POST requests ask the server to process some provided data. Most commonly, in Traffic Ops, this means creating an object based on the serialization of said object contained in the request body, but it can also be used virtually whenever no other method is appropriate. When an object *is* created, the response body MUST contain a representation of the newly created object. POST requests do not need to be *idempotent*, unlike PUT requests.
+
+PUT
+---
+PUT is used to replace existing data with new data that is provided in the request body. :RFC:`2616#section-9.1.2` lists PUT as an "idempotent" request method, which means that subsequent identical requests should ensure the same state is maintained on the server. What this means is that a client that PUTs an object representation to Traffic Ops expects that if they then GET a representation of that object, do the same PUT again and GET another representation, the two retrieved representations should be identical. Effectively, the ``lastUpdated`` field that is common to objects in the :ref:`to-api` violates this, but the other properties of objects - which can actually be defined - generally obey this restriction. In general, fulfilling this restriction means that handlers will need to require the entirety of an object be defined in the request body.
+
+When an object is replaced, the response body MUST contain a representation of the object after replacement. While :RFC:`2616` states that servers MAY create objects for the passed representations if they do not already exist, :ref:`to-api` endpoint authors MUST instead use POST handlers for object creation.
+
+All endpoints that support the PUT request method MUST also support the :mailheader:`If-Unmodified-Since` HTTP header.

Review comment:
       Yes: [one of them is real, and the other isn't](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers#Conditionals)
   
   I get that wrong all the time too. In fact, it was wrong in the original blueprint until Rob pointed it out.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org