PUT requests removes required fields with default values

[smyrman]

If I PUT a new resource at /resourcepath/myid, It will set non-provided required fields with the specified Default values. If I do another PUT request against the same URL with the same body, required fields that are not part of the body gets cleared away...

I would instead have expected one of:

1. The request to be rejected with 422 due to missing required fields.
2. The request to fill in the missing fields with default values, so that calling a PUT request twice has the same effect as calling it once.

I believe 2 is the most correct / desirable thing to do, or to put it in other words; the least wrong thing to do. This would still "reset" fields that are not in the body to their default values, but that seams acceptable, and in some cases even desirable. It must of course be documented that this is what happens.

Case 1 is also much better than the current behaviour, but can not guarantee that a second PUT request will work just because the first one did, which if nothing else, is vert convenient.

Rest-layer versions:

• 326f807
• rs/rest-layer-mem@25a468b
• rs/rest-layer-mongo@544bb6a

Relevant references:

• Section 9.6 PUT in https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.htm

[smyrman]

For 0.2, do what ever is simplest. E.g. rejecting with 422 is fine for now if that's easier.

[Dragomir-Ivanov]

HTTP PUT is expected to be idempotent, so it makes sense calling it multiple times to return the same result ( filling the missing required with default values ). I will look around.

[Dragomir-Ivanov]

I think this should go as follows:

1. User makes initial PUT request, all required and not supplied values get their default values.
2. User makes second PUT request, rest-layer doesn't attempt to delete any required value but leaves the old value, because since the field is required, it needs to be there.

We document that behavior in the README. Does it sounds plausible @smyrman ?

[smyrman]

I think it's important not to mimic PATCH behaviors, so I think it probably should not matter if a field is required or not for weather or not the original value is kept.

I think that for most cases, what makes sense is to error if a required field without a default is missing, and to always set defaults for all fields, no matter if they are required or not. There are however some odd cases, and I will get back to at least one of those.

Consider a schema (pseudo notation):

• name, string, required
• type, string, default=y

For a sequence of PUT requests, this are the responses/results I think make the most sense:

>> PUT resource/1 {"name": "x"}
<< 201 Created {"id": 1, "name": x", "type": "y"}
>> PUT resource/1 {"name": "x"}
<< 200 OK {"id": 1, "name": "x",  "type": "y"}
>> PUT resource/1 {}
<< 422 OK {"code": 422, "message":"Document contain error(s)",  "issues": {"name":["required"]}}

Or if we are passing a type the first time:

>> PUT resource/1 {"name": "x", "type": "z"}
<< 201 Created {"id": 1, "name": x",  "type": "z"}
>> PUT resource/1 {"name": "x"}
<< 200 OK {"id": 1, "name": x",  "type": "y"} // type is reset to default

But now, I will come to the difficult part... What about we adding a field "createTime"?

new schema:

• name, string, required
• type, string, defalut=y
• createTime, initHook=time.Now(), read-only

In this speical-case I want the createTime to be kept.

>> PUT resource/1 {"name": "x", "type": "z"}
<< 201 Created {"id": 1, "name": x",  "type": "z", "createTime": "2018-02-02T22:41:14.0054Z"}
>> PUT resource/1 {"name": "x"}
<< 200 OK {"id": 1, "name": x",  "type": "y",  "createTime": "2018-02-02T22:41:14.0054Z"}

So to sum that up, I think the best semantics are:

• All fields except read-only fields gets "reset".
• Defaults are set on update as well as insert for PUT requests.
• Errors are returned on missing required fields for updating PUT request, just as they would for insert

Ther may be more edge cases we are missing, but it would be a lot better than the current behaviour. The next big question is, how easy is it to implement this the current code base.

As a limitation of scope, I don;t think we can't easily solve this for sub fields of nested FieldValidators atm. (schema.Dict, schema.AnyOf, schema.AllOf, schema.Object); Those probably need to be addressed by #192 and #77. Longer term we should probably support at least AnyOf and AllOff clauses... But if we can solve it at the top level, that would be a great start never the less.

For our own code, we have disabled PUT (via middleware), so that we can prevent users from being able to clear fields that they should not be allowed to clear according to the schema. If the fields are used as part of determining access, that would be a potential attack surface.

[smyrman]

I also need to add, that a perfectly valid solution for now, at least as far as I am concerned, could be not to allow PUT request to do an update at all; they are allowed as a method of creating new objects, but for UPDATE, they return a HTTP 501 Not Implemented.

That addresses any security and data-corruption concerns, and people can still do updates via PATCH.

[Dragomir-Ivanov]

That addresses any security and data-corruption concerns, and people can still do updates via PATCH.

I agree with that, but it is a REST convention, that you can update the whole document with PUT, where any missing fields, are deleted from original document(unlike PATCH, where missing fields stay as is in the original document).
I am planning to implement JSON PATCH protocol for PATCH method, where we will be able to delete fields on PATCH, but still PUT is expected to work.

[Dragomir-Ivanov]

For me, (Required, Default) fields are not possible. If not, what is the difference between (Required,Default) and just (Default). To me on PUT, Required fields must always be present, or the request fails.

[smyrman]

I don't disagreee @Dragomir-Ivanov; I am just stating that removing PUT support until we are able to fix it (if there is no easy fix for it now) would be a better fix than to do nothing for a v0.2 tag release. Anyway, if you have time to have a look at how to address this properly, that would be the best.

On (Required, Default) v.s. (Default), the difference probably is that it would be valid to clear a field with a Default only value (e.g. via #145 or directly in code when using the schema and resource packages directly) - if it's not ReadOnly of course. Other than that, they are semantically the same for the case of PUT.

Basically, if checking Required after setting the Default for all missing fields (required or not), it would be correct in my mind... Does it make sense?