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

feat: export input and output schemas to components in OpenAPI document #131

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

EnriqueL8
Copy link
Contributor

@EnriqueL8 EnriqueL8 commented Feb 29, 2024

Draft PR until this is merged getkin/kin-openapi#914. Note: the go.mod is pointing to a local version as you cannot reference a fork in go.mod

This PR extends the OpenAPI handler to export the input and output schemas into the OpenAPI components section to reduce the size of the OpenAPI documents generated and reduce duplication.

By default, it will not export generic schemas as such as thing does not exist in the OpenAPI specification as far as I know and will export the top level schema for the route. You can configure the latter to not export it.

There is a point of friction with the schema customizers, currently we have two schema customisers that look for the annotations ffexcludeinput and ffexcludeoutput. The way a schema is added to the Components section is by name of the Golang Struct. If a route or multiple route are defined where the response type is used across both inputs and outputs where we have used the above exclude annotations, then the schema will be overridden by the last route parsed. This is not ideal and the only solution I could think of is to enforce consumers to define two different structs/pojos but this renders the above annotations mostly useless. So any thoughts appreciate!

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
// Validate doesn't like references
// so LoadFromData resolves those references
// before we validate
err = doc.Validate(sl.Context)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nguyer
Copy link
Contributor

nguyer commented Feb 29, 2024

I really like the direction this is going. @EnriqueL8 do you have a small example of two structs that would be problematic with the name conflict you mentioned above? Do we have any that would hit this conflict in use today?

@EnriqueL8
Copy link
Contributor Author

@nguyer yes, getkin/kin-openapi#914 (comment)

I'll check if we have any today in our repositories

@EnriqueL8
Copy link
Contributor Author

Going to pull this into a Firefly draft branch and see how it behaves!

@EnriqueL8
Copy link
Contributor Author

That revealed an issue where you get empty names for the schemas so you end up with

 $ref: '#/components/schemas/'

Will look into that!

@EnriqueL8
Copy link
Contributor Author

Oh it fails when the field is for example Options map[string]interface{} ffstruct:"ContractDeployRequest" json:"options"` so it's thinking a map is an object I should export, interesting.... We should only export struct, so this needs a fix into PR in kin-openapi

@EnriqueL8
Copy link
Contributor Author

Alright put a fix into the PR in the openapi package!

Apart from that all I see is when validating it

Errors: 
	-paths.'/apis/{apiName}'. Declared path parameter apiName needs to be defined as a path parameter in path or operation level
	-paths.'/namespaces/{ns}/identities/{did}'. Declared path parameter did needs to be defined as a path parameter in path or operation level
	-paths.'/namespaces/{ns}/apis/{apiName}'. Declared path parameter apiName needs to be defined as a path parameter in path or operation level
	-paths.'/identities/{did}'. Declared path parameter did needs to be defined as a path parameter in path or operation level

Need to check for the duplicate objects problem

@EnriqueL8
Copy link
Contributor Author

EnriqueL8 commented Mar 4, 2024

Just documenting as I go... we have this in some paths /{did:did:.+} which some weird regex annotation. Need to figure out this gets translated into OpenAPI

@EnriqueL8
Copy link
Contributor Author

If this PR is consumed by Firefly , then the Node.JS SDK should also be updated with the new OpenAPI

@EnriqueL8
Copy link
Contributor Author

EnriqueL8 commented Mar 14, 2024

If just this the problem with the annotations overriding a struct with https://github.com/hyperledger/firefly/blob/577e8c47680c6230209a74829921a9c427766af8/pkg/core/message.go#L76C71-L76C91

and came across https://swagger.io/docs/specification/data-models/data-types/ read only and write only which is exactly what we want! We don't want to ignore the property just return it on GET but not on POST for example. Will contribute this here so we can pick up the component schemas and the behaviour stays the same for the annotations

@EnriqueL8
Copy link
Contributor Author

Actually this will still have the same problem, I wonder if I need to implement a merge instead of replace of component schemas?

@peterbroadhurst
Copy link
Contributor

@EnriqueL8 - this seems like an important change. Are you still progressing?

I see that getkin/kin-openapi#914 and getkin/kin-openapi#937 got to a conclusion.

@EnriqueL8
Copy link
Contributor Author

Need to come back to this one and make sure that we do not break existing ffexclude annotations as we add this. My worry is that there is significant rework across each repo when this changes occur

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants