Updated the specification file and reformated all the patches incrementaly

xavierlacot commented 3 years ago

Hi, I updated the openapi spec file and reapplied/updated/cleanup all the patches. Please note that I did not check in Slack doc if the patches are useful or not.

I noticed that some of them have been merged in the official Slack openapi file, which lead me to remove the patch 9 and slightly reduce the size of the patch 10.

damienalexandre commented 3 years ago

Looks good to me, the only change I found strange is:

-        $optionsResolver->setRequired([]);
+        $optionsResolver->setRequired(['token']);

On almost all endpoints it seems (not all, like ConversationsCreate.php).

It's true that the token is required: https://api.slack.com/methods/emoji.list#arg_token but that's supposed to be handled by our Client Factory: https://github.com/jolicode/slack-php-api/blob/28967f79835df4266da5d18979b0a78986a7d62f/src/ClientFactory.php#L42

You should never have to send to token yourself and now it's mandatory in the endpoint options.

That's not your fault, the spec changed:

Before

                "operationId": "emoji_list",
                "parameters": [
                    {
                        "description": "Authentication token. Requires scope: `emoji:read`",
                        "in": "query",
                        "name": "token",
                        "type": "string"
                    }
                ],

Now

                "operationId": "admin_emoji_list",
                "parameters": [
                    {
                        "description": "Set `cursor` to `next_cursor` returned by the previous call to list items in the next page",
                        "in": "query",
                        "name": "cursor",
                        "type": "string"
                    },
                    {
                        "description": "Authentication token. Requires scope: `admin.teams:read`",
                        "in": "query",
                        "name": "token",
                        "required": true,
                        "type": "string"
                    },
                    {
                        "description": "The maximum number of items to return. Must be between 1 - 1000 both inclusive.",
                        "in": "query",
                        "name": "limit",
                        "type": "integer"
                    }
                ],

What the documentation says

From https://api.slack.com/web

Authenticate your Web API requests by providing a bearer token, which identifies a single user, bot user, or workspace-application relationship. Register your application with Slack to obtain credentials for use with our OAuth 2.0 implementation, which allows you to negotiate tokens on behalf of users and workspaces. We prefer tokens to be sent in the Authorization HTTP header of your outbound requests. However, you may also pass tokens in all Web API calls as a parameter called token.

How to fix?

We have two choices:

:a: accept the PR as is, breaking our promise on the Client Factory, adding a "token" parameters "sometimes"
:b: add a new patch that remove the "required" part of the "token" parameter to allow using the Client as easily as before

I'm in favor of :b: - what do you think?

xavierlacot commented 3 years ago

Yes, :b: seems a better solution (from a DX point of vue). I am going to work on the changes.

xavierlacot commented 3 years ago

Another point, which is quite related: it was a pain to reformat the patches. Very often, the patch tool looses himself trying to figure out which line to change... and it fails, applying changes where is shouldn't, etc. This makes the current process hardly maintainable in the long term, I would say.

However, there are two things we could do to make patches more "robust":

sort the openapi spec file by keys, recursively. This will not change the meaning of the spec, and will ensure that the keys always come in the same order, even if Slack decides to wrangle the file paths order.
add more context in patches. At the moment, lines like 10-more-completeness.patch#L96-L97 are not applied at the right position, because the patch tool misses context. We can add more context lines above and below a diff by passing an additional -U15 parameter to git diff, which should help. Combined with the first point, it should make the patching process more robust, I believe.

If you agree, I would add in this pull request:

a CLI command that would download the original vanilla spec, turn into an alphabtically sorted equivalent, then apply the patches and generate the slack-openapi-patched.json file
a new "reset" patch equivalent to the current patches, but applicable on the sorted vanilla spec.
a cli command that would regenerate this patch file base on the current diff between slack-openapi-patched.json and slack-openapi.json. This will give us a single patch file, but versionned, which will allow to track the patch changes when Slack updates the vanilla spec.

This way, the next time a patch has to be generated:

the developer updates the vanilla spec using a CLI command
the developer makes his changes in the generated patched spec file (which is sorted by keys)
the developer regenerates the patch using a CLI command
the developer generates the SDK and commits all the changes

What would you thing about this?

damienalexandre commented 3 years ago

I like this :+1: but I fear having only one patch file will make it hard to read "what is changed from the official spec". Having multiple files allowed us to be incremental - but as you said it's a pain to update. :+1: for your proposal.

We can also switch to the new "no example" specification: https://github.com/slackapi/slack-api-specs/blob/master/web-api/slack_web_openapi_v2_without_examples.json - that way exemples will not get in our way when changed / edited.

xavierlacot commented 3 years ago

Yay :muscle: @damienalexandre and @pyrech can you take a look at it?

The commit f6d15613983f0c49f5afa3fcffe0482d436d3cc0 exactly shows what we earn when a change is made in te spec: mainly a diff in the patches file, which is quite readable. Of course, we'll still have a detailed review to undergo if Slack decides to change all the format if the spec, but it should a lot more stable now.

xavierlacot commented 3 years ago

:pray: thanks for the feedback on the documentation

damienalexandre commented 3 years ago

Tests are green, let's move this to master!

Release is postponed as we are waiting for another improvement (Jane upgrade) :+1:

jolicode / slack-php-api