[FAB-18315] Ch.Part.API: Create Swagger definition for API

[stephyee]

Type of change

• Documentation update

Description

Generated swagger specification using the go-swagger annotations.

Additional details

1. Install goswagger (if not already installed)
2. Generate json swagger generate spec -o ./swagger/swagger-fabric.json --scan-models --exclude-deps --input swagger/tags.json
3. View using Swagger Editor

💭 I think the spec generation could be automated Added scripting for this

Related issues

FAB-18302

[jyellick]

A few top level questions:

1. How are you generating the swagger json from the code? It might be obvious, and I have my suspicions, but there are multiple tools. We should minimally have a script, possibly with Makefile integration to regenerate these definitions.
2. Did you look into some way to namespace this or specify by server? There are obviously multiple servers, including the operations endpoint, as well as the main grpc server. It would probably be confusing to users simply seeing this as a global top level definition.

[stephyee]

@jyellick

How are you generating the swagger json from the code? It might be obvious, and I have my suspicions, but there are multiple tools. We should minimally have a script, possibly with Makefile integration to regenerate these definitions.

For this change, I generated the swagger manually using the go-swagger tool from the annonations I included in the code. Like you, even I noted the spec generation could be automated - I didn't do it here.

Did you look into some way to namespace this or specify by server? There are obviously multiple servers, including the operations endpoint, as well as the main grpc server. It would probably be confusing to users simply seeing this as a global top level definition.

No I didn't. I'd have to look into it more. IIUC with the swagger tool used it'd have to be on per server or grouped (how the channels now).

[jyellick]

@stephyee

Thanks for updating with the generation bits!

No I didn't. I'd have to look into it more. IIUC with the swagger tool used it'd have to be on per server or grouped (how the channels now).

I think this is something we should definitely investigate.

Have you tried using these definitions? Ideally, we'd include some sort of test to verify that they have been specified properly, but minimally I'd think we'd want some sort of sniff test that they can be consumed to use some function of the API.

[stephyee]

@jyellick

Have you tried using these definitions? Ideally, we'd include some sort of test to verify that they have been specified properly, but minimally I'd think we'd want some sort of sniff test that they can be consumed to use some function of the API.

If specified incorrectly, the generation will fail.

// swagger:operati DELETE /channels/{channelID} channels deleteChannel

Running the script with the above will return classifier: unknown swagger annotation "operat".

[wlahti]

Forgive me if some of these wording changes weren't written by you and instead generated from the code/comments. :)

[wlahti]

@denyeart @jyellick @btl5037 Now that 2.3.0 has been released, can we go ahead and merge this PR and refine things further as necessary over this release cycle? I don't see any outstanding review comments that need to be addressed.

[jyellick]

@stephyee

If specified incorrectly, the generation will fail.

// swagger:operati DELETE /channels/{channelID} channels deleteChannel

Running the script with the above will return classifier: unknown swagger annotation "operat".

I'm more concerned about the definition referencing a 'DELETE' when it should be a 'PATCH', or a typo in the URL path portion etc. I would certainly expect the swagger generation to catch malformed swagger doc, but not malformed does not imply correct.

@wlahti

@denyeart @jyellick @btl5037 Now that 2.3.0 has been released, can we go ahead and merge this PR and refine things further as necessary over this release cycle? I don't see any outstanding review comments that need to be addressed.

Do we have issues open for the remaining items? Particularly I'm concerned with the two I raised earlier:

1. Can we give these definitions some server namespacing?
2. How do we know we can actually build a client that works using these definitions? Ideally we'd have some sort of automated testing around it, but even a one-off would make me feel more comfortable.

[stephyee]

@jyellick

I'm more concerned about the definition referencing a 'DELETE' when it should be a 'PATCH', or a typo in the URL path portion etc. I would certainly expect the swagger generation to catch malformed swagger doc, but not malformed does not imply correct.

Ah, currently this would fall to the author and reviewers to ensure the annotations are up-to-date. It's not ideal but swagger is only a specification. Of course, tooling exists to validate request/responses against a swagger spec (i.e. swagger-request-validator). We'd have to decide if adding more tooling is worth the investment.

Do we have issues open for the remaining items? Particularly I'm concerned with the two I raised earlier:

1. Can we give these definitions some server namespacing?
2. How do we know we can actually build a client that works using these definitions? Ideally we'd have some sort of automated testing around it, but even a one-off would make me feel more comfortable.

1. I forget to mention I did test "namespacing". How does this look?
   
2. I looked into this a little then my 🤯 . I was successful generating the client but using it with a running server was were I encountered issues. I opened FAB-18372 for it. Update I manually confirmed client generation and usage - see updated issue.

[jyellick]

1. I forget to mention I did test "namespacing". How does this look?

Certainly it looks nice to have the operations and channel participation APIs broken out. But since this is a top level Fabric artifact, do we need to worry about server namespacing? My concern here is that someone will end up trying to use the channel participation APIs against the peer for instance, though perhaps my concerns there are overblown.

2. I looked into this a little then my  . I was successful generating the client but using it with a running server was were I encountered issues. I opened FAB-18372 for it. Update I manually confirmed client generation and usage - see updated issue.

It's great to get at least some basic validation that it's possible to build a client which works for some operation using the swagger definition. Looking at your branch, it certainly is a lot of additional tooling and code, so I agree that it's not obvious we want to pull of that in. I hate advertising support for a feature, and having no way to tell if we break it (beyond the obvious syntactical failures), but if it would be too onerous to integrate, I suppose we might have to skip it this time. Still, I feel much better about integrating this with the manual testing done.

Obviously we've just gone through the US holiday season, so that's somewhat to blame for my tardiness, but I'm sorry to have kept this outstanding so long @stephyee. I think it makes sense to merge this now, and we can leave issues open for any remaining items.

[jyellick]

The build check around the JS analysis looks broken, but perhaps rebasing and force pushing will clear it up?

[lindluni]

The build check around the JS analysis looks broken, but perhaps rebasing and force pushing will clear it up?

I already looked into them. It's incorrectly identifying code, we don't have an LGTM config file so it tries to do its best to match code types and correctly compile. It was a miss in this case and can be ignored.

[stephyee]

@jyellick

Certainly it looks nice to have the operations and channel participation APIs broken out. But since this is a top level Fabric artifact, do we need to worry about server namespacing? My concern here is that someone will end up trying to use the channel participation APIs against the peer for instance, though perhaps my concerns there are overblown.

I misunderstood your comment before, you concern is not overblown. You mean some APIs are for the orderer but not the orderer etc.. As a temp solution I can add to the descriptions to specify which server supports specific endpoints. We can open a separate issue to investigate namespacing servers. Also I added external doc links which will clarify the API usage.

It's great to get at least some basic validation that it's possible to build a client which works for some operation using the swagger definition. Looking at your branch, it certainly is a lot of additional tooling and code, so I agree that it's not obvious we want to pull of that in. I hate advertising support for a feature, and having no way to tell if we break it (beyond the obvious syntactical failures), but if it would be too onerous to integrate, I suppose we might have to skip it this time. Still, I feel much better about integrating this with the manual testing done.

Fair points. If we could minimize the dependencies and code used to validate client generation then it'd be worthwhile to have a build check for it.

I'm going to rebase but like Brett says, the build check is an invalid here.
This PR isn't exactly straightforward so thanks for all the reviewing pre and post holidays.