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.

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

RFC: gluon-config-api #2296

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

lemoer
Copy link
Member

@lemoer lemoer commented Aug 12, 2021

This is my suggestion to support a REST API for the config-mode.

It basically works like this:

  • A single JSON object called "config" is exposed via http.
  • Packages can add config definitions in /lib/gluon/config-api/parts/*.lua.
    • Such a file contains a json-schema for their config options,
    • a method to get the config,
    • and a method set the config.
  • The complete config definition is merged from all snippets in /lib/gluon/config-api/parts/*.lua.
  • The json-schema verification is based on libucl.

Here is an example usage. To get the config, use the following command:

$> curl http://localhost:6080/cgi-bin/api/v1/config -X GET
{
   "wizard": {
     "contact": "Leo",
     "location": {
       "lon": 9.7192900000000009,
       "lat": 52.386696000000001,
       "share_location": false
     }
   }
 }

To get the complete schema, use:

#> curl http://localhost:6080/cgi-bin/api/v1/config -X OPTIONS
{
   "schema": {
     "type": "object",
     "additionalProperties": false,
     "properties": {
       "wizard": {
         "properties": {
           "contact": {
             "type": "string"
           },
           "location": {
             "type": "object",
             "required": [
               "lat",
               "lon",
               "share_location"
             ],
             "properties": {
               "lon": {
                 "type": "number"
               },
               "lat": {
                 "type": "number"
               },
               "share_location": {
                 "type": "boolean"
               }
             }
           }
         },
         "additionalProperties": false,
         "type": "object"
       }
     },
     "required": [
       "wizard"
     ]
   },
   "allowed_methods": [
     "GET",
     "POST",
     "OPTIONS"
   ]
 }

Change contact from Leo to Hans:

#> curl http://localhost:6080/cgi-bin/api/v1/config -X POST --data @- <<EOI
{
   "wizard": {
     "contact": "Hans",
     "location": {
       "lon": 9.7192900000000009,
       "lat": 52.386696000000001,
       "share_location": false
     }
   }
 }
EOI
{
   "status": 200,
   "error": "Accepted"
 }

See the result:

#> curl http://localhost:6080/cgi-bin/api/v1/config -X GET
{
   "wizard": {
     "contact": "Hans",
     "location": {
       "lon": 9.7192900000000009,
       "lat": 52.386696000000001,
       "share_location": false
     }
   }
 }

Feel free to comment this approach.


(This is based on the ideas in #2238)

@github-actions github-actions bot added the 3. topic: package Topic: Gluon Packages label Aug 12, 2021
@lemoer
Copy link
Member Author

lemoer commented Dec 27, 2021

This has not yet received a lot of attention. Can I provide additional information? Is there anything missing?

@neocturne
Copy link
Member

Some unsorted thoughts:

  • I like the JSON-based data structure
  • Having the ability to download configuration as a whole is probably generally a good thing for performance. While I'd love to have some GraphQL-like filtering capabilities, the increased processing time would likely more than outweigh the lower transferred data volume
  • I'm not a fan of JSON schemas, but I guess I can live with it if it works well for our use case.
    • What's the binary size of libucl?
    • How are config schema definitions combined?
    • See also https://gitlab.com/neoraider/ece/-/wikis/design/schema, which was the simplified/modified JSON schema approach of my GSoC project a few years ago - in particular it was designed to allow merging of modular schemas. Use a more maintained library like libucl is definitely preferable to my old code though.
  • We should not abuse the OPTIONS method, there should be a separate schema URL you can GET
  • Is deleting values possible, or do we expect that no values are optional and must be set to "null" when they don't have a value?
  • Should modifying array values be possible in an atomic way, or do you need to GET + POST the whole array? Should a way to detect or prevent concurrent changes be provided?

I have not looked at the implementation yet.

@lemoer
Copy link
Member Author

lemoer commented Jan 25, 2022

Some unstructured answers...

What's the binary size of libucl?

IIRC, libucl is ~51kb. I decided to use it for my RFC, since it worked more or less without much effort.

How are config schema definitions combined?

I used (more or less) the merging algorithm that you developed for ece.

See also https://gitlab.com/neoraider/ece/-/wikis/design/schema, which was the simplified/modified JSON schema approach of my GSoC project a few years ago ...

Yes. I am aware of that code and looked into it. As it uses blobs instead of json, I decided not to use it as of now. Also I think, some parts were even unfinished.

@lemoer
Copy link
Member Author

lemoer commented Jan 26, 2022

  • Is deleting values possible, or do we expect that no values are optional and must be set to "null" when they don't have a value?
    Should modifying array values be possible in an atomic way, or do you need to GET + POST the whole array?
  • Should a way to detect or prevent concurrent changes be provided?

In the current version of this RFC, there is no patching of the old config based on the POST request. The new config is actually directly the body of the POST request. So there is no merging of the old config with the new config.

This means that if you want to change just a single value, you have to retrieve the entire config and post it again.

I know this is unperformant and ugly (especially for larger applications), but I think it could be useful for this API:

  • It's easy to understand for users of the API.
  • The code stays simple. (The old state is irrelevant. No merge between old and new state necessary).

@lemoer
Copy link
Member Author

lemoer commented Jan 26, 2022

Btw. an example of how a config could look like can be found here: Link

@lemoer
Copy link
Member Author

lemoer commented Nov 21, 2022

@NeoRaider This is lying around for over a year without any feedback. Is there any progress?

@AiyionPrime AiyionPrime added the 2. status: waiting-on-review Awaiting review from the assignee but also interested parties. label Jan 8, 2023
@AiyionPrime AiyionPrime requested a review from neocturne January 8, 2023 14:28
Copy link
Member

@AiyionPrime AiyionPrime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

takeaways

  • frontend and backend should be able to verify against the same schema
  • should still be possible to have config objects depending on installed packages (like batman-adv, or community specific packages)
  • have a look at gluon-web-model for current implementation of data attributes and logic
  • way to go:
    • go step by step.
    • gluon-web-model already provides information about dependencies/type/....
    • expose this as API.
    • try to build on this.
  • javascript code should reference fields from backend.
    • based on the information about dependencies/type/..., the frontend code could render a field.
  • Most of the model definition could be reused.
    • The biggest problem is, that fields are not well defined by their identifiers.
    • One of the first steps would be to replace the enumeration of forms and sections.

@lemoer will also have a look at OpenAPI / swagger.

end
end

-- TODO: implement array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

found it

@@ -0,0 +1,16 @@
# Copyright (C) 2021 Leonardo Moerlein <me at irrelefant.net>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spdx

@@ -0,0 +1,16 @@
# Copyright (C) 2021 Leonardo Moerlein <me at irrelefant.net>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spdx

@@ -0,0 +1,51 @@
#
# Copyright (C) 2021 OpenWrt.org
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spdx no further license comments


PKG_SOURCE_PROTO:=git
PKG_SOURCE_URL:=https://github.com/vstakhov/libucl
PKG_SOURCE_VERSION:=e6c5d8079b95796099693b0889f07a036f78ad77
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AiyionPrime AiyionPrime added 2. status: waiting-on-author Waiting on some action from the author and removed 2. status: waiting-on-review Awaiting review from the assignee but also interested parties. labels Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. status: waiting-on-author Waiting on some action from the author 3. topic: package Topic: Gluon Packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants