RCP-010 - Add/Edit Endorsement

darnjo commented 3 years ago

The goal of this issue is to create an Add/Edit Endorsement, meaning creating a specification for it. We also need to move what's in Confluence before we do that, as much of the spec is already written, it just needs to be updated to reflect the current work in Transport and Certification.

Documentation and Specification

[x] Make initial copy from Confluence Add/Edit specification. Don't port RCP-011 from the following section.
[x] Finish the draft Add/Edit testing rules, and review in the workgroups. See GitHub Discussion

Tool Development

[ ] Write testing tools (RESO Commander)
[ ] Create report format for Certification
[ ] Add Reporting options to Cert site

codygustafson commented 1 year ago

I have a lot to review here with this endorsement. It has been quite some time since we have last looked at it. On the call today we discussed errors and error messages so I thought I would start with that. I'll paste the example I found at the following link and describe what I would like to see changed below the example. https://reso.atlassian.net/wiki/spaces/WebAPI2/pages/8163004691/2.9.+Errors+and+Warnings

{
  "error": {
    "code": "20100",
    "message": "Update Field Errors",
    "target": "Update",
    "details": [
      {
       "code": "30212",
       "target": "Property.ListPrice", 
       "message": "List Price too Low"
      },
      {
       "code": "30327,
       "target": "Property.SalePrice", 
       "message": "Sale Price too High"
      }
    ],
    "innererror": {
      "trace": [...],
      "context": {...}
    }
  }
}

    "message": "Update Field Errors",

This top level message does not seem useful to me. There may be a reason to have a message for the entire property instead of each individual field but that doesn't seem to be reflected here.

    "code": "20100",

RESO should likely reserve a code-range to be used for standard errors at some point in the future. Should this be an integer instead of a string?

    "target": "Update",

Not sure what use this target has. What is "Update"? Is it just letting us know we used a PUT or PATCH? I would think this target would either be the item you are modifying, perhaps "Property" or "Request".

    "details": [

I feel like this should be "PropertyErrors" - something that specifically notes that this is in reference to the fields or properties of what you submitted.

       "target": "Property.ListPrice",

I don't see a purpose of us referencing this in terms of the metadata. I'd much rather reference this in terms of the request that was made. Example "ListPrice", "Media". The reference of what this object is doesn't seems very relevant to the client. I would rather reference what they sent us.

On the call I mentioned the thought of "which media item is invalid". I do think that is needed. Something such as

...
     "PropertyErrors": {
       "code": "123456",
       "target": "Media", 
       "index": 0, 
       "PropertyErrors": {
            "code": "789101112",
            "target" "Order"
            "message": "Order cannot be null",
        }
      },
...

    "innererror": {
      "trace": [...],
      "context": {...}
    }

I don't understand what this section is attempting to show. Can we expand or remove it?

codygustafson commented 1 year ago

2.8. Additional Update Features

This section references batch requests, change sets and async requests. Which of these did we say we were ditching on the call?

codygustafson commented 1 year ago

2.7.2 Mandatory Actions

The actions described in this section MUST be supported by all servers.

OrderChildren looks like a MUST define in the precious spec. I doubt we want to proceed with that, thoughts?

codygustafson commented 1 year ago

@darnjo, I don't know where the MUST/SHOULDs you referenced in the meeting are documented at. Could you send me a link when you get a chance?

darnjo commented 1 year ago

The OData “4.01” specification outlines payload error responses here: OData Version 4.01. Part 1: Protocol

The representation of an error response body is format-specific. It consists at least of the following information:

· code: required non-null, non-empty, language-independent string. Its value is a service-defined error code. This code serves as a sub-status for the HTTP error code specified in the response.

· message: required non-null, non-empty, language-dependent, human-readable string describing the error. The Content-Language header MUST contain the language code from [RFC5646] corresponding to the language in which the value for message is written.

· target: optional nullable, potentially empty string indicating the target of the error, for example, the name of the property in error.

· details: optional, potentially empty collection of structured instances with code, message, and target following the rules above.

· innererror: optional structured instance with service-defined content.

Service implementations SHOULD carefully consider which information to include in production environments to guard against potential security concerns around information disclosure.

Since the target property has been used to convey the source of the error, the goal would be to find a way to address items within that structure.

For example, a listing’s second media item could be: “target": "Property.Media[1]". This would work for more deeply nested elements too, if needed. I'm not a huge fan of coming up with another addressing scheme, but as long as it's lightweight and simple perhaps it's not an issue.

Regarding batch, async, and child order actions, these were removed from the spec and are not represented in the testing rules, which can be found here: https://github.com/RESOStandards/transport/discussions/41

The MUST items outlined on the November Certification call can be found there as well. We need to determine which MUSTs we want to test for.

dgmyrek commented 1 year ago

Unless I'm misinterpreting the spec/tests, it looks like servers MUST support DELETE requests on any field they advertise as editable. A server should instead have the ability to denote which resources (or even fields) support which actions (POST/PATCH/DELETE) for the following reasons:

There are instances where MLS customers wish to grant write access to certain records or field subsets of those records. They don't wish to trust the operating party with full control over all fields and/or the ability to DELETE records.
On the server side, some resources simply can't support DELETE in the conventional sense, yet the server provider may wish for these resources to still be POST/PATCH capable.

dgmyrek commented 1 year ago

Unless I'm misinterpreting the spec/tests, it looks like servers MUST support DELETE requests on any field they advertise as editable. A server should instead have the ability to denote which resources (or even fields) support which actions (POST/PATCH/DELETE) for the following reasons:

There are instances where MLS customers wish to grant write access to certain records or field subsets of those records. They don't wish to trust the operating party with full control over all fields and/or the ability to DELETE records.

On the server side, some resources simply can't support DELETE in the conventional sense, yet the server provider may wish for these resources to still be POST/PATCH capable.

Josh helped set me straight.

Add/Edit providers may implement as many or as few actions as makes sense for their business case, with one or more standard or local fields.

I had been searching for a "MAY" or "MUST" in the doc, but this line indicates that DELETE is indeed optional. Disregard my last comment.

darnjo commented 1 year ago

Thanks for the update, Dylan. Good catch and questions on the requirements. Please feel free to raise any other questions you might have.

There are certain things that OData requires in terms of its conformance levels, such as DELETE as you noticed, but that RESO will not be testing for in the Add/Edit Endorsement.

We've tried to keep the requirements as light as possible.

RESOStandards / transport

RCP-010 - Add/Edit Endorsement #4