Refactor /set-field endpoint

[dcastro]

Clarification and motivation

The set-field endpoints are defined as:

    :<|> "set-field"
      :> RequiredParam "path" (QualifiedPath EntryPath)
      :> RequiredParam "field" FieldName
      :>
        (    "private" :> Post '[JSON] Entry
        :<|> "public"  :> Post '[JSON] Entry
        :<|> ReqBody '[JSON] (Maybe FieldContents)
          :> Post '[JSON] Entry
        )

In other words, we have 3 distinct endpoints:

• /set-field with an optional Maybe FieldContents body
• /set-field/private with no body
• /set-field/public with no body

There are multiple issues with this interface:

1. The body of the /set-field endpoint is Maybe FieldContents, which suggests it's okay to call it with null in the body. But this doesn't make sense. If the ?field name already exists, then calling /set-field with null in the body will do absolutely nothing. If the field does not yet exist, /set-field will fail:

   $ curl -s -D /dev/stderr -H 'token: root' -X POST -H 'Content-Type: application/json' \
     'localhost:8081/api/v1/content/set-field?path=/entry&field=fff' \
     -d 'null' | jq
   
   HTTP/1.1 400 Bad Request
   Transfer-Encoding: chunked
   Date: Thu, 02 Jun 2022 15:43:29 GMT
   Server: Warp/3.3.18
   Content-Type: application/json;charset=utf-8
   
   [
     {
       "error": "The entry at '/entry' does not yet have a field 'fff'.\nIn order to create a new field, please include 'FIELDCONTENTS' in the body.",
       "code": 301
     }
   ]
   

   So there's no use case for this.
2. To create a private field, we need two separate calls:
   • POST /set-field with the field contents in the body
   • POST /set-field/private

These issues can be solved by having a single endpoint that accepts an optional ?visibility=public/private query parameter and an optional FieldContents.

We should also be able to inline handleSetFieldResult as part of this issue.

Acceptance criteria

We have a single /set-field endpoint

[sancho20021]

@dcastro by optional FieldContents you mean ReqBody '[JSON] FieldContents? But if it is null, the problem remains the same:

• do nothing if a field already exists
• error if a field doesn't exist

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.

[dcastro]

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:

• Split the CLI's set-field command in two:
  • set-field, which has a required FIELDCONTENTS argument, and an optional -V argument for convenience
  • set-field-visibility, which has a required VISIBILITY argument.
    This way you can set one argument, or the other, or both, but never neither.
• Do the same in the Web API, /set-field and /set-field-visibility.

[sancho20021]

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.

[dcastro]

Separate PRs sounds good (you can link them both to this issue)