Open dcastro opened 2 years ago
@dcastro by optional FieldContents you mean ReqBody '[JSON] FieldContents
? But if it is null, the problem remains the same:
The combination visibility=null && fieldcontents=null is a bit strange, I think that is the main issue, which won't be resolved with your suggestion. Or probably I misunderstood something.
the problem remains the same:
Well, depends on how you look at it. The problem I described was that there was no use case for setting fieldcontents to null. With my proposed change, there would be a use case: if you set the fieldcontents to null and the visibility to not null, you would change the visibility of the field and leave its contents intact.
But you're right, there would still be the issue that "the combination visibility=null && fieldcontents=null is a bit strange".
It's the same thing with the CLI's set-field
command, where both arguments are optional. coffer set-field /dir/entry user
will do nothing if the user
field exists and crash if it doesn't.
So perhaps we should:
set-field
command in two:
set-field
, which has a required FIELDCONTENTS
argument, and an optional -V
argument for convenienceset-field-visibility
, which has a required VISIBILITY argument.
This way you can set one argument, or the other, or both, but never neither./set-field
and /set-field-visibility
.Sounds OK, I assume that CLI's set-field
should be done in a separate issue. I will do that for the Web-API, and after that open a new one for the CLI if everything goes right.
Separate PRs sounds good (you can link them both to this issue)
Clarification and motivation
The
set-field
endpoints are defined as:In other words, we have 3 distinct endpoints:
/set-field
with an optionalMaybe FieldContents
body/set-field/private
with no body/set-field/public
with no bodyThere are multiple issues with this interface:
The body of the
/set-field
endpoint isMaybe FieldContents
, which suggests it's okay to call it withnull
in the body. But this doesn't make sense. If the?field
name already exists, then calling/set-field
withnull
in the body will do absolutely nothing. If the field does not yet exist,/set-field
will fail:So there's no use case for this.
POST /set-field
with the field contents in the bodyPOST /set-field/private
These issues can be solved by having a single endpoint that accepts an optional
?visibility=public/private
query parameter and an optionalFieldContents
.We should also be able to inline
handleSetFieldResult
as part of this issue.Acceptance criteria
We have a single
/set-field
endpoint