Rest api rework #124

DenysGonchar · 2020-05-27T08:01:08Z

This PR replaces an old cowboy-swagger based REST API with the new openapi-generator implementation.

The generated server code is in the separate repo https://github.com/esl/amoc_rest

The whole migration process is described here in details

NB: it is important to merge this PR using "create a merge commit" option!!!

rpc:multicall() doesn't guarantee the sequence of returned Results so running lists:zip with Nodes list and Results is not safe.

….yaml file. this is done only to simplify 'git rebase', no functional changes.

…e '/upload' method

…on interface

michalwski

Thanks for the PR, it's good to see another approach to providing correct and up to date API spec. It looks like you put much effort to make this work.

Being honest, I'm currently not convinced that's a convenient way to go. I may not have the full understanding yet or don't see the real value. Below there are my concerns:

I can see that we have multiple static files describing the API
- open_api/amoc-openapi.yml
- open_api/amoc-swagger.json
- open_api/amoc_rest/priv/openapi.json
Can we have only one?
As far as I understand the approach taken in this PR generates the erlang code from the API spec. I find the other way around (the spec generated from the code) more convenient.
The auto-generated code doesn't fit our (any?) formatting rules

open_api/amoc_rest/rebar.config

DenysGonchar · 2020-05-27T16:59:17Z

1. I can see that we have multiple static files describing the API
   
   * `open_api/amoc-openapi.yml`
   * `open_api/amoc-swagger.json`
   * `open_api/amoc_rest/priv/openapi.json`
   
   Can we have only one?

yes the whole open_api directory will be removed in the last commit, it is there just to keep the history of migration from amoc-swagger to openapi-generator in git. The only file we need is amoc-openapi.yml. there is also priv/openapi.json file - the generated json version of the yaml file, we should not edit this json file manually.
you can find more information here

2. As far as I understand the approach taken in this PR generates the erlang code from the API spec. I find the other way around (the spec generated from the code) more convenient.

the main problem with that spec generation approach - it's almost impossible to keep everything up to date. generation of the spec is not ideal, it's hard to support JSON schemas and validation of input and output must be done manually. it's based on cowboy-trails project which is not well maintained, as well as cowboy-swagger. also both projects are purely documented.

3. The auto-generated code doesn't fit our (any?) formatting rules

ideally, this generated code should be used as is, w/o any changes. It also can be moved out to the separate rebar3 project and used in amoc as a dependency, but from my perspective this is not necessary.

….com/esl/amoc_rest/

michalwski

Thanks for extracting the generated code to an external repository. It's much cleaner and easier to understand it now. I had some questions and comments on the code.

michalwski · 2020-06-08T07:40:59Z

src/amoc.app.src

        exometer_core,
        exometer_report_graphite,
-        exometer_report_statsd,
-        ssl,


I'd like to understand why amoc doesn't need the the ssl application now.

amoc is a framework and it doesn't establish any connections. ssl must be mentioned as a dependency for the apps that really use it.

src/rest_api/amoc_api.erl

src/rest_api/amoc_api_logic_handler.erl

michalwski · 2020-06-08T08:29:45Z

src/rest_api/amoc_api_logic_handler.erl

+            Users = maps:get(<<"users">>, Body),
+            SettingsMap = maps:get(<<"settings">>, Body, #{}),
+            Settings = [begin
+                            Key = binary_to_atom(K, latin1),


This is probably a copy-paste. I think it'd be good to change latin1 to utf8 as we have in other projects (MongooseIM for instance). It'd be even better to use binart_to_existing_atom. The perfect solution would be to parse the binary and return atom only when the binary is acceptable.

yes, I tried to introduce changes step by step. so the whole implementation of the rest api handlers is extracted from the previous cowboy-swagger based implementation. the risk is quite low, taking into account that this API should be called just once during the whole amoc app lifecycle. And the goal of this PR is to have equivalent API after migration, while binart_to_existing_atom usage requires introduction of some new error code. Also, amoc_config_env:parse_value(V) can still introduce new atoms, but we definitely don't want to change that functionality.

michalwski · 2020-06-08T08:38:38Z

src/rest_api/amoc_api_logic_handler.erl

+    {200, #{}, [{<<"nodes">>, ResponseList}]};
+handle_request('ScenariosGet', _Req, _Context) ->
+    Scenarios = amoc_scenario:list_scenario_modules(),
+    BinaryScenarios = [atom_to_binary(S, latin1) || S <- Scenarios],


Is there a reason behind the latin1 encoding in amoc? Maybe utf8 could be used here?

latin1 should be good enough for the module name, but I can change it to utf8 if you want.

michalwski · 2020-06-08T08:40:40Z

src/rest_api/amoc_api_logic_handler.erl

+            {200, #{}, [{<<"compile">>, Error}]}
+    end;
+handle_request(OperationID, Req, Context) ->
+    error_logger:error_msg(


In amoc we are using the built-in Erlang logger. I think it'd be better to be consistent with that and use it here as well.

sure, that is a copy-past from the default handler. thanks for catching that.

src/rest_api/amoc_api_scenario_status.erl

src/rest_api/amoc_api_upload_scenario.erl

michalwski · 2020-06-08T08:42:48Z

test/amoc_api_scenario_handler_SUITE.erl

@@ -59,17 +57,6 @@ end_per_testcase(
 end_per_testcase(_, _Config) ->
    destroy_env().

-get_scenario_status_returns_200_when_scenario_exists(_Config) ->


Why this test case was removed?

it's a very old one. given_test_status_mocked(true) - this is completely invalid, the mocked function should never return true value (even before the migration), loaded is used instead.

michalwski · 2020-06-08T08:44:19Z

test/amoc_api_status_handler_SUITE.erl

@@ -43,15 +31,6 @@ returns_up_when_amoc_up_online(_Config) ->
    ?assertEqual(200, CodeHttp),
    ?assertMatch({[{<<"node_status">>, <<"up">>}]}, Body).

-
-returns_down_when_api_up_and_amoc_down_offline(_Config) ->


Why this test case was removed?

it doesn't introduce any value. it tests absolutely the same thing asreturns_down_when_api_up_and_amoc_down_online, but restricts internal implementation.

DenysGonchar added 24 commits May 26, 2020 18:57

changing scenario uploading path to '/scenarios/upload'

14820ff

improve compilation errors reporting

bc5ea15

rpc:multicall() doesn't guarantee the sequence of returned Results so running lists:zip with Nodes list and Results is not safe.

generating amoc-openapi.yaml

0409c44

move '/scenarios/upload' definition to the bottom of the amoc-openapi…

65926ba

….yaml file. this is done only to simplify 'git rebase', no functional changes.

validating amoc-openapi.yaml

39c390f

generating erlang stub server

d366220

removing cowboy-swagger based REST API server

67741b8

inegrating erlang stub server

6ec1fc1

swagger-ui integration. downloading 'dist' version of swagger-ui

33db85a

swagger-ui integration. copying swagger-ui into the priv folder

0c34238

swagger-ui integration. verifying changes to swagger-ui

cc9972a

swagger-ui integration. adding cowboy static routing for swagger-ui

0f27fa9

fixing minor dialyzer errors in erlang stub server

978a257

implementing REST API. dummy logic handler module

176c811

implementing REST API. HTTP GET methods implementation

a6e919f

implementing REST API. HTTP GET methods schema update

d9fece0

implementing REST API. OpenAPI definition update for '/upload' method

bf466a7

implementing REST API. '/upload' method implementation

b6120f8

implementing REST API. 'application/octet-stream' content type for th…

afc2652

…e '/upload' method

implementing REST API. update '/upload' method description

8746369

implementing REST API. update OpenAPI definition for scenario executi…

14603f5

…on interface

implementing REST API. implement scenario execution interface

c099ba1

fixing xref errors in erlang stub server

0814228

fixing mocking and 'undefined functions' issues

03227c0

DenysGonchar added the WIP Work In Progress label May 27, 2020

michalwski reviewed May 27, 2020

View reviewed changes

open_api/amoc_rest/rebar.config Outdated Show resolved Hide resolved

DenysGonchar added 4 commits May 27, 2020 12:54

use named json schemas in 'amoc-openapi.yaml'

4f5046f

fixing amoc_api_node_handler_SUITE

471cb7e

fixing amoc_api_scenario_handler_SUITE

faf9d40

fixing amoc_api_scenarios_handler_SUITE

32b8244

updating REST API descriptions in 'amoc-openapi.yaml'

732de77

DenysGonchar force-pushed the rest-api-rework branch from abe50d8 to 79a9b27 Compare May 27, 2020 16:08

updating REST API documentation

7f73d73

DenysGonchar force-pushed the rest-api-rework branch from 79a9b27 to 7f73d73 Compare May 27, 2020 16:24

DenysGonchar force-pushed the rest-api-rework branch from 4b35f19 to 4bbe8b6 Compare May 28, 2020 00:04

DenysGonchar added 3 commits May 28, 2020 02:07

added readme files for ./priv and ./src/rest_api directories

782e3d9

removing open_api directory

2974e77

updating cowboy version to 2.7.0

f967538

DenysGonchar force-pushed the rest-api-rework branch from 4bbe8b6 to f967538 Compare May 28, 2020 00:08

adding CI verification of OpenAPI spec

50cc0e5

DenysGonchar force-pushed the rest-api-rework branch from cef87d8 to 50cc0e5 Compare May 29, 2020 09:09

DenysGonchar added 2 commits June 8, 2020 02:21

moving generated amoc_rest server into a separate repo https://github…

f6c14e4

….com/esl/amoc_rest/

removing unneeded amoc dependencies

1a76792

DenysGonchar force-pushed the rest-api-rework branch from 9e416ac to 1a76792 Compare June 8, 2020 00:21

michalwski reviewed Jun 8, 2020

View reviewed changes

updating copyright information in the source files

a3fe768

DenysGonchar force-pushed the rest-api-rework branch from cfc6f63 to e1d839d Compare June 8, 2020 18:21

changes according to the review comments

3e9fb1d

DenysGonchar force-pushed the rest-api-rework branch from e1d839d to 3e9fb1d Compare June 8, 2020 18:29

michalwski approved these changes Jun 9, 2020

View reviewed changes

updating amoc_rest dependency

b27596b

DenysGonchar force-pushed the rest-api-rework branch from 83cfe36 to b27596b Compare June 9, 2020 13:30

michalwski merged commit e4566b6 into master Jun 9, 2020

DenysGonchar removed the WIP Work In Progress label May 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rest api rework #124

Rest api rework #124

DenysGonchar commented May 27, 2020 •

edited

michalwski left a comment

DenysGonchar commented May 27, 2020 •

edited

michalwski left a comment

michalwski Jun 8, 2020

DenysGonchar Jun 8, 2020

michalwski Jun 8, 2020

DenysGonchar Jun 8, 2020

michalwski Jun 8, 2020

DenysGonchar Jun 8, 2020

michalwski Jun 8, 2020

DenysGonchar Jun 8, 2020

michalwski Jun 8, 2020

DenysGonchar Jun 8, 2020

michalwski Jun 8, 2020

DenysGonchar Jun 8, 2020

Rest api rework #124

Rest api rework #124

Conversation

DenysGonchar commented May 27, 2020 • edited

michalwski left a comment

Choose a reason for hiding this comment

DenysGonchar commented May 27, 2020 • edited

michalwski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DenysGonchar commented May 27, 2020 •

edited

DenysGonchar commented May 27, 2020 •

edited