split http#410
Conversation
eaacd85 to
0cce433
Compare
duglin
left a comment
There was a problem hiding this comment.
reviewed with group
| @@ -857,6 +897,9 @@ attributes. However, they MUST adhere to the following rules: | |||
|
|
|||
| #### Common Attributes | |||
|
|
|||
| TODO is it better to have this section, or to just define them fully | |||
| upon first usage and then have all other point to that first usage? | |||
| The Resource's document is base64 encoded and serialized as a string. | ||
|
|
||
| - `<RESOURCE>url`: this attribute MUST be used when the Resource is stored | ||
| - `<RESOURCE>url`: this attribute MUST be used when the document is stored | ||
| external to the Registry. Its value MUST be a URL that can be used to | ||
| retrieve its contents via an HTTP(s) `GET`. There is no requirement for the |
| @@ -1115,6 +1170,9 @@ of the existing entity. Then the existing entity would be deleted. | |||
| This specification does not place any constraints on the data returned from | |||
| an HTTP `GET` to this URL. | |||
|
|
|||
| TODO do we want to keep the HTTP-ness of this URL? And 'icon' attr too. | |||
| deprecated.docs too | |||
| updated. A value of `null` MUST generate an error | ||
| ([invalid_data](#invalid_data)). | ||
|
|
||
| TODO - null->err or erase? |
| @@ -2363,18 +1722,21 @@ The following defines the specification-defined capabilities: | |||
| - It is STRONGLY RECOMMENDED that implementations support at least | |||
| `/capabilities` and `/model`. | |||
|
|
|||
| TODO are these values HTTP specific? Perhaps not call this | |||
| "apis", but instead be called "readable" and remove the leading "/" ? | |||
| And then include "entities" in the list to align with "mutable" below? | |||
|
|
||
| It is expected that only certain clients (e.g. "admin" users) would be | ||
| allowed to edit this attribute. So, for "normal" users the "SHOULD" | ||
| statements are expected to be "MUST" statements. |
There was a problem hiding this comment.
s/statements/statement/
SHOULD NOT
|
|
||
| See: [`?binary` Flag](./http.md#binary-flag) for the HTTP usage. | ||
|
|
||
| TODO do we want references like this? |
| would return the same subset of entities. | ||
| expression(s) such that a query to that URL would return the same subset of | ||
| entities. | ||
| TODO ^^^ what if the protocol isn't http? |
There was a problem hiding this comment.
check the protocol to see if the filter flag appears in the URL or not
There was a problem hiding this comment.
add link to http spec
| @@ -5765,19 +3616,62 @@ those cases, the client will need to query the individual inlineable attributes | |||
| in isolation so the Registry can leverage | |||
| [pagination](../pagination/spec.md) of the response. | |||
|
|
|||
| TODO ^^^ ref to pagination | |||
|
|
||
| Regardless of the mechanism used to update the capabilities, the Registry's | ||
| `epoch` value MUST be incremented. | ||
| #### Offered Capabilities |
There was a problem hiding this comment.
make this /capabilitiesoffered
4cb0e72 to
4d44343
Compare
Signed-off-by: Doug Davis <duglin@gmail.com>
| Additionally, this specification defines the core model and semantics of | ||
| xRegistry, while secondary specifications (such as | ||
| [xRegistry HTTP Binding](./http.md)) will discuss how to expose them in a | ||
| protocol specific way. |
There was a problem hiding this comment.
| protocol specific way. | |
| protocol-specific way. |
| @@ -60,7 +55,7 @@ A Registry consists of two main types of entities: Resources and Groups of | |||
| such Resources. | |||
|
|
|||
| Resources typically represent the main data of interest in the Registry, while | |||
| Groups, as the name implies, allow related Resources to be arranged together | |||
| Groups, as the name implies, allows related Resources to be arranged together | |||
There was a problem hiding this comment.
I think it was right as it was, because if you drop what's in between the comma, it would be "allow"
| accessed through the Versions collection. | ||
| A Resource entity in the Registry holds one or more Versions of metadata, and | ||
| optionally a domain-specific document. If a Resource holds multiple Versions, | ||
| those can be organized with compatibility policies and lineage. Each Resource |
There was a problem hiding this comment.
Let's link to compatibility and ancestor here?
|
|
||
| #### Document Resources vs Metadata-Only Resources | ||
|
|
||
| Each Version of a Resource MAY be defined to have a "domain-specific" document | ||
| associated with it. These documents MAY be stored within the Version as an | ||
| attribute, or MAY be stored external to the Version and a URL to its location | ||
| will be stored within the Version instead. This model design choice is | ||
| specified via the `hasdocument` aspect of the Resource's model definition. | ||
| specified via the | ||
| [`hasdocument` aspect](./model.md#groupsstringresourcesstringhasdocument) |
There was a problem hiding this comment.
You can ignore this to a later point, but do we differentiate between attribute and aspect, and why and when? I think hasdocument is consistently referred to as an aspect, but not sure why the differentiation is made.
There was a problem hiding this comment.
I've been trying to refer to "model attributes" as "aspects"... sort of like another word for "feature".
Then I use "attribute" for user-facing attributes in the data model (group, resources...).
But perhaps there's no point?
| This specification is designed such that clients can choose how they want the | ||
| data from a server to be returned. There are three main "views" that clients |
There was a problem hiding this comment.
| This specification is designed such that clients can choose how they want the | |
| data from a server to be returned. There are three main "views" that clients | |
| This specification is designed such that clients can choose how they want to interact with the Registry's data. There are three main interaction models that clients |
Just a suggestion
There was a problem hiding this comment.
Let's chat tomorrow because we do use the terms "document view" and "api view" - so I was trying to (sort of) relate to those too
There was a problem hiding this comment.
sticking with "view"
| The above requirement is driven by the idea that modifying the capabilities | ||
| of a server and modifying any entity data are typically two very distinct | ||
| actions, and will not normally happen at the same time. However, if the | ||
| situation does occur a consistent (interoperable) processing order needs to |
There was a problem hiding this comment.
| situation does occur a consistent (interoperable) processing order needs to | |
| situation does occur, a consistent (interoperable) processing order needs to |
| ``` | ||
| - Document View Constraints: | ||
| - REQUIRED. | ||
| - If the `meta` entity is inlined in the document then this attribute |
There was a problem hiding this comment.
| - If the `meta` entity is inlined in the document then this attribute | |
| - If the `meta` entity is inlined in the document, then this attribute |
|
|
||
| #### Resource Attributes | ||
| When the xRegistry metadata is serialized as a JSON object, the processing | ||
| of the 3 Version `<RESOURCE>` attributes MUST follow these rules: |
There was a problem hiding this comment.
Can we make this Version-level, and add a * after <RESOURCE to clarify exactly what is meant?
| used. | ||
|
|
||
| An `xref` value that points to a non-existing Resource, either because | ||
| it was deleted, never existed or the current client does not have permission |
There was a problem hiding this comment.
Shouldn't the permission case be handled differently? I agree for the other situations, but accessing a resource that you're not authorized to see would give you a NotAuthorised in most other environments, which is helpful to understand why it can't be found, and now, there are multiple possible reasons it can't be found that may have to be validated somehow by the user. Why can't this be handled differently?
I know this was already here and you moved it around, but now I reread it and this thought came up.
There was a problem hiding this comment.
Let's discuss tomorrow, but they do have access to the Resource with the "xref" (the source), so they can always "GET" that one. If they don't have access to the target of the "xref" I'm not sure they should get a "401 Not Authorized" on a GET to the source.
It seems safest to treat a "missing target" the same as "you don't have permission to see the target".
From a security perspective, even saying "the target exists but you can access it" is a security issue to some folks because it makes hackers aware that "something" is even there.
| Versions of Messages. Non-leaf parts of the `<PATH>` MUST only reference | ||
| xRegistry collection names and not any specific entity IDs since `<PATH>` is | ||
| meant to be an abstract traversal of the model. | ||
| The value of the `inline` flag is list of `<PATH>` values where is a string |
There was a problem hiding this comment.
| The value of the `inline` flag is list of `<PATH>` values where is a string | |
| The value of the `inline` flag is a list of `<PATH>` values where is a string |
There was a problem hiding this comment.
Actually something is wrong at the end too, but not sure how to fix it 😅
There was a problem hiding this comment.
See if this is better:
The value of the `inline` flag is a list of `<PATH>` values where each is a
string indicating which inlineable attribute to show in the response.
Signed-off-by: Doug Davis <duglin@gmail.com>
| ### No-Code Servers | ||
|
|
||
| In the [core specification](./spec.md) there is a | ||
| disussion about ["no-code servers"](./spec.md#design-no-code-servers). In the |
There was a problem hiding this comment.
good catch! the spell checker wasn't turned on for this file!
Signed-off-by: Doug Davis <duglin@gmail.com>
Signed-off-by: Doug Davis <duglin@gmail.com>
jskeet
left a comment
There was a problem hiding this comment.
I like the separation of HTTP details from the spec. The TOC for http.md looks good, but I haven't read through all the details (it's a lot!) - was this a matter of "lift and shift" or was there rewriting which would be worth reviewing in more detail?
|
Mainly "lift and shift".
that would be much appreciated. |
jskeet
left a comment
There was a problem hiding this comment.
Can't say this is comprehensive by any means, but a reasonably time-boxed review has at least thrown up some things for discussion. Happy to be overruled on any of them, as normal.
| - [Error Processing](#error-processing) | ||
| - [Events](#events) | ||
| - [Version Entity](#version-entity) | ||
| - [Flags](#flags) |
There was a problem hiding this comment.
This currently links to the flags bit of capabilities. Renaming the section to "Request flags" would resolve this issue and make it clearer. (Even before trying to click through, the title felt incongruous to me.)
| - [Sort Flag](#sort-flag) | ||
| - [SpecVersion Flag](#specversion-flag) | ||
| - [Error Processing](#error-processing) | ||
| - [Events](#events) |
There was a problem hiding this comment.
I'm not sure this deserves its own top-level section, given that it just links elsewhere. The text could potentially be inlined directly, or it could be a subsection - maybe of "Registry Design".
There was a problem hiding this comment.
I definitely see your point. I think the main reason I put it as a top-level section is because if someone was coming to the spec to see the list of events I didn't want the section to be hidden in the middle of the TOC. Let's see what other people think.
There was a problem hiding this comment.
AI: move into "design" section
| - [`PUT /`](#put-) | ||
| - [`POST /`](#post-) | ||
| - [`GET /export`](#get-export) | ||
| - [Registry Capability](#registry-capability) |
There was a problem hiding this comment.
Possibly rename to Registry Capabilities, given that all requests operate on the collection?
| - [`?filter` Flag](#filter-flag) | ||
| - [`?ignoredefaultversionid` Flag](#ignoredefaultversionid-flag) | ||
| - [`?ignoredefaultversionsticky` Flag](#ignoredefaultversionsticky-flag) | ||
| - [`?ignoreeopch` Flag](#ignoreepoch-flag) |
There was a problem hiding this comment.
| - [`?ignoreeopch` Flag](#ignoreepoch-flag) | |
| - [`?ignoreepoch` Flag](#ignoreepoch-flag) |
| HTTP/1.1 204 No Content | ||
| ``` | ||
|
|
||
| ## Flags / Query Parameters |
There was a problem hiding this comment.
Do we need all the detail here? I wonder whether just saying "Flags are represented by query parameters of the same name" would be enough (and keep a link to the flags in the spec, of course). Otherwise whenever a new flag is added or changed, we'll have to update two places.
If there are aspects to handling of some specific flags that are HTTP-specific, I think those could be called out here, only where necessary.
There was a problem hiding this comment.
Glad you focused on this - I was wonder about it.
I like your idea of just calling out the few flags that might need extra text. I'll hold off making the change until the other xReg folks chime in just in case there's a reason to keep the 1:1 mapping.
|
|
||
| A server MAY support clients creating or updating a | ||
| [Version](./spec.md#version-entity) of a | ||
| [Resource](./spec.md#resource-entity) via an HTTP `PATCH` or `POST` directed |
There was a problem hiding this comment.
Should POST be PUT here, as per the section title? POST is described later.
| { ... Contents of a schema doc excluded for brevity ... } | ||
| ``` | ||
|
|
||
| #### `PATCH /<GROUPS>/<GID>/<RESOURCES>/<RID>/versions/<VID>`] |
There was a problem hiding this comment.
Not sure what these ] end-of-line characters are doing here
There was a problem hiding this comment.
I think those used to have some flags listed at the end. Done
| [`hasdocument`](./model.md#groupsstringresourcesstringhasdocument) | ||
| is set to `false`), then use of the `$details` suffix in a request URL MUST | ||
| be treated the same as if it were absent and any URLs in server response | ||
| messages referencing that Resource (e.g. `self`) MUST NOT include it. |
There was a problem hiding this comment.
This feels odd to me. It means that if a resource changes from not having a domain-specific document to having one, a response can change from metadata to document... rather than changing from error to document.
This may all be ancient history and decided by now, but it feels to me like if I ask for something (the document) and that's not supported, that should give an error, rather than a different result.
To put it another way - suppose we'd made the metadata the default, and had $document as a suffix when requesting a document, would we want an explicit $document suffix request to return the metadata if there was no document?
There was a problem hiding this comment.
We may want to add some background on this to the primer since other people may wonder about it too, but....
switching between having a domain-specific doc and not is not something that will happen often. It's kind of a big decision point in how someone wants to store their data in the registry. Once that decision is made we had to consider each of those two types of users and what they would expect to be "default" for GET. For the non-domain-specific doc users it's easy since there's no choice... they'll get the metadata.
However, for a user with a domain-specific doc we decided that the information that's most important to them is that doc, not the xReg metadata. We did consider a $document type of flag (for consistency as you suggested) but then consider what a schema user would see/use:
GET https://example.com/schemagroups/group1/schemas/myschema.json$document
yuck! Kind of kills the usability factor. So we ended up where we are.
This is related to why we return the scalar metadata as xRegistry- HTTP headers... just in case they wanted some of the metadata too, but really the doc is still the main reason for the GET. This allows xReg-aware clients and non-xReg-aware client to use the same URL.
I will point out though, we purposely support the idea of $details always being allowed on a Resource/Version URL so that clients shouldn't be required to check the hasdocument flag for each Resource prior to doing a "give me the metadata vie" request.
Does this make sense?
There was a problem hiding this comment.
I wasn't actually proposing using a $document suffix - I was just trying to introduce it as a thought experiment for how reasonable it is for $details to be effectively optional for any resource that doesn't support a document.
Fundamentally, it feels like this approach reduces predictability. If I see a URL which doesn't have $details, I don't know whether that will return the document or the metadata, unless I know whether the resource supports documents or not.
I suspect it's worth discussing in the meeting - or simply overruling the objection as "this has been thrashed out a lot in the past", which is totally valid.
There was a problem hiding this comment.
Fundamentally, it feels like this approach reduces predictability. If I see a URL which doesn't have $details, I don't know whether that will return the document or the metadata, unless I know whether the resource supports documents or not.
Yup that's true, and to some extent it's done on purpose. As i mentioned, we wanted the naked URL to always be "the data that's most important to THAT client". And the answer to that question changes based on whether the Resource type has a doc or not. So, here, "usability for client of THIS Resource type" takes precedence over "consistency between doc-Resources and non-doc-Resources".
Let's discuss on the call
| When a Resource is serialized as its underlying domain-specific document, | ||
| in other words `$details` is not appended to its URL path, the HTTP body of | ||
| requests and responses MUST be the exact bytes of that document. If the | ||
| document is empty, or there is no document, then the HTTP body MUST be empty |
There was a problem hiding this comment.
I think we need to be clear about the difference between "there is no document" and "the resource type doesn't support documents". They can be easily confused, and we're saying they have very different meanings and results.
There was a problem hiding this comment.
This paragraph isn't dealing with "the resource doesn't support docs" though. Do I need to make that clearer or were you just talking about the overall spec in general?
There was a problem hiding this comment.
I was saying that just a paragraph ago, we're talking about an attribute of hasdocument which really sounds like it's similar to "does this resource have a document" but isn't. Now we're talking about "does this resource have a document" which is different, and I think the two concepts can be confused really easily.
There was a problem hiding this comment.
ok - let me see if I can work on that...might not be until after the call though
There was a problem hiding this comment.
If right before this paragraph (so at the start of this section), I added this:
This section goes into more details concerning situations where a Resource
type is defined to support domain-specific documents (i.e.
[`hasdocument`](./model.md#groupsstringresourcesstringhasdocument)
is set to `true`.
would that have helped?
There was a problem hiding this comment.
You mentioned that hasdocument might not be as clear as it could be. See if I used this as the first paragraph of the previous section would help:
In the core specification's
[Resource Metadata vs Resource Document](./spec.md#resource-metadata-vs-resource-document)
section, it explains how Resources types might be defined to optionally have a
domain-specific document associated with them via their
[`hasdocument` model aspect](./model.md#groupsstringresourcesstringhasdocument`)
being set to `true`. For HTTP, clients indicate whether they want to interact
with the Resource's xRegistry metadata or the Resource's domain-specific
document by the use of a `$details` suffix on the `<RESOURCE>id` in its URL.
Presence of the `$details` suffix MUST be interpreted as a request to interact
with the xRegistry metadata of the Resource, while its absence MUST be
interpreted as a request to interact with the Resource's domain-specific
document.
There was a problem hiding this comment.
Quite possibly. I think naming the attribute "supportsdocuments" instead of "hasdocument" would probably be even clearer, but that ship's sailed :)
Possibly where we mention empty serialization of a resource where the type supports documents but there isn't one for the specific resource, it's worth adding a sentence to highlight that this isn't the same as the resource type not supporting documents.
There was a problem hiding this comment.
hmm supportsdocuments is clearer. We'd need to discuss with the rest of the team but I don't think it's too late to rename it. "hasdoc" is so much easier to say though :-)
There was a problem hiding this comment.
You mentioned that hasdocument might not be as clear as it could be. See if I used this as the first paragraph of the previous section would help:
I believe I understand that. Basically there are three possibilities:
- The model's
hasdocumentis true, and the resource itself has a document attached - The model's
hasdocumentis true, and the resource itself doesn't have a document attached - The model's
hasdocumentis false, so the resource itself can't have a document attached
What I'm saying is that those last two are pretty easily confused. "There is no document" sounds - at first glance - like hasdocument=false, right? But one is on the model, one is on the resource. I think it's okay as a design, but we need to be aware of an audience who isn't completely tuned in to that.
|
|
||
| Since only some types of attributes can appear as HTTP headers, in order | ||
| to manage the full set of attribute the xRegistry metadata view (via use | ||
| of the `$details` URL suffix) MUST be used instead. |
There was a problem hiding this comment.
Unless the resource doesn't support a document, in which case a request without a $details suffix should apparently be treated the same way as if $details was specified? (This is an example of why I think that rule is unhelpful.)
There was a problem hiding this comment.
You have that correct. Let's discuss on the call given my explanation in a previous comment.
Signed-off-by: Doug Davis <duglin@gmail.com>
|
@jskeet thanks so much for the review! A few good topics to discuss on today's call, the other changes are in the PR. |
| This URL MUST NOT include the `$detail` suffix even if the Resource's | ||
| `hasdocument` aspect is set to `true`. | ||
| Note that if the Resource's | ||
| [`hasdocument` aspect][./model.md#groupsstringresourcesstringhasdocument) |
There was a problem hiding this comment.
| [`hasdocument` aspect][./model.md#groupsstringresourcesstringhasdocument) | |
| [`hasdocument` aspect](./model.md#groupsstringresourcesstringhasdocument) |
(The link is broken at the moment)
Signed-off-by: Doug Davis <duglin@gmail.com>
Signed-off-by: Doug Davis <duglin@gmail.com>
|
Approved on the 9/25 call. |
fixes #409