Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor /set-field endpoint #119

Open
dcastro opened this issue Jun 2, 2022 · 4 comments · May be fixed by #130
Open

Refactor /set-field endpoint #119

dcastro opened this issue Jun 2, 2022 · 4 comments · May be fixed by #130
Assignees
@sancho20021
Labels
web-api This issue is related to the Coffer Web API

Comments

@dcastro
Copy link
Member

dcastro commented Jun 2, 2022 •
edited
Loading

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
@dcastro dcastro added the web-api This issue is related to the Coffer Web API label Jun 2, 2022
@sancho20021
Copy link
Contributor

@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
Copy link
Member Author

dcastro commented Jul 24, 2022

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
Copy link
Contributor

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
Copy link
Member Author

dcastro commented Jul 24, 2022

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

sancho20021 added a commit that referenced this issue Jul 24, 2022
@sancho20021
[#119] Change set-field Web-API
a56bbda 
Problem: `set-field` route was used both for changing visibility
and setting fields. This resulted in ambiguous behavior in case
of missing contents and visibility options.

Solution: require contents for setting field and allow changing
visibility for convenience. Add `set-field-visibility` request
which doesn't require field contents.
@sancho20021 sancho20021 mentioned this issue Jul 24, 2022
Closed
5 tasks
sancho20021 added a commit that referenced this issue Jul 26, 2022
@sancho20021
[#119] Add test for set-field-visibility
f9c5fa6
sancho20021 added a commit that referenced this issue Jul 28, 2022
@sancho20021
[#119] Remove JSON body from set-field-visibility
4a373e2
sancho20021 added a commit that referenced this issue Aug 1, 2022
@sancho20021
[#119] Add set-field-visibility to CLI and refactor set-field
48bd50e 
Problem: See issue #119.

Solution: Split `set-field` command into a `set-field` with contents
and `set-field-visibility`.
sancho20021 added a commit that referenced this issue Aug 1, 2022
@sancho20021
[#119] Add set-field-visibility to CLI and refactor set-field
1ae2034 
Problem: See issue #119.

Solution: Split `set-field` command into a `set-field` with contents
and `set-field-visibility`.
sancho20021 added a commit that referenced this issue Aug 1, 2022
@sancho20021
[#119] Add set-field-visibility to CLI and refactor set-field
01f4190 
Problem: See issue #119.

Solution: Split `set-field` command into a `set-field` with contents
and `set-field-visibility`.
@sancho20021 sancho20021 linked a pull request Aug 1, 2022 that will close this issue
Open
5 tasks
sancho20021 added a commit that referenced this issue Aug 2, 2022
@sancho20021
[#119] Refactor and new test for missing contents in set-field
72d11b0
sancho20021 added a commit that referenced this issue Aug 2, 2022
@sancho20021
[#119] Refactor and new test for missing contents in set-field
e5e088d
sancho20021 added a commit that referenced this issue Aug 2, 2022
@sancho20021
[#119] Add set-field-visibility to CLI and refactor set-field
13fa4f7 
Problem: See issue #119.

Solution: Split `set-field` command into a `set-field` with contents
and `set-field-visibility`.
sancho20021 added a commit that referenced this issue Aug 2, 2022
@sancho20021
[#119] Edit docs and README
6bd6412
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sancho20021 sancho20021

Labels
web-api This issue is related to the Coffer Web API
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[#119] Refactor set-field in CLI and WebAPI serokell/coffer
2 participants
@dcastro @sancho20021