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

encoding/opeanpi: paths parsing for OpenAPI spec #1727

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

qequ
Copy link

@qequ qequ commented May 20, 2022

Implemented parsing for paths in cue when interpreting OpenAPI.

parsing functionality of most of the objects of a path as given by swagger spec;

  • For a Path Item object;
    • description,
    • all operations (get, post, put, etc).
  • For a Operation object
    • Description
    • Responses
    • Security
  • For a Response Object
    • description
    • content
  • For a Media Type Object
    • schema

Parsing notes;
Instead of using a single paths top declaration object defined "Paths" as suggests #386 each path is written separately as a top decl object withe the prefix " $/ ", similarly as Components Objects are defined using definitions.

If Security is defined as a Path Item object, that Security list will be applied to all the operations

For parsing Media Type Objects schemas, Components Objects schemas parsing functionality was reused

Added unit tests
All tests passed (running go test ./...)

Fixes #386 #735

qequ added 30 commits April 19, 2022 15:11
…rations

now the paths are checked with a prefix of the form $/ instead of $
now checking with a regex with words of the form (...)/(...)
…rations

now the paths are checked with a prefix of the form $/ instead of $
now checking with a regex with words of the form (...)/(...)
@qequ qequ requested a review from cueckoo as a code owner May 20, 2022 21:10
}

return ast.NewList(items...)
//pb.path.Set("security", ast.NewList(items...))
Copy link

Choose a reason for hiding this comment

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

remove commented code


}

//rb.mediaTypes.Set("description", description)
Copy link

Choose a reason for hiding this comment

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

remove commented code

@myitcv myitcv added the Discuss Requires maintainer discussion label May 23, 2022
@araddon
Copy link

araddon commented Jun 12, 2022

I started hacking against this pr and looking at original tickets and thought i would leave what i learned:

  • This does not cover the openapi[yaml,json] deserialize -> non-interpreted cue -> simplified cue decoding (which i was looking for) . both the 386, 735 tickets are relatively ambiguous if they cover that but mostly refer to the cue -> openapi[yaml,json] encoding .
  • As i was hacking the package namespacing became an issue, the encoding/jsonschema contains the bulk of the decode code now, but it appears the desire to separate the jsonschema vs openapi into two packages. Since none of the openapi specific decoding (paths, security, etc) is implemented I ran into either needing jsonschema/decode.go decoder, state objects to be public or the need to put the bulk of the openapi objects -> cue into jsonschema/decode.go, constraint.go . My rough thoughts were the bulk of the code in decoder, state relates to translating generic cue ast (from json/yaml deserialization) into simpler cue objects, and had little to do with either spec and thus need to share single package.

@dgutson
Copy link

dgutson commented Sep 6, 2022

I started hacking against this pr and looking at original tickets and thought i would leave what i learned:

  • This does not cover the openapi[yaml,json] deserialize -> non-interpreted cue -> simplified cue decoding (which i was looking for) . both the 386, 735 tickets are relatively ambiguous if they cover that but mostly refer to the cue -> openapi[yaml,json] encoding .
  • As i was hacking the package namespacing became an issue, the encoding/jsonschema contains the bulk of the decode code now, but it appears the desire to separate the jsonschema vs openapi into two packages. Since none of the openapi specific decoding (paths, security, etc) is implemented I ran into either needing jsonschema/decode.go decoder, state objects to be public or the need to put the bulk of the openapi objects -> cue into jsonschema/decode.go, constraint.go . My rough thoughts were the bulk of the code in decoder, state relates to translating generic cue ast (from json/yaml deserialization) into simpler cue objects, and had little to do with either spec and thus need to share single package.

@qequ pls address this

@qequ
Copy link
Author

qequ commented Sep 13, 2022

@araddon the only feature I worked on in this PR was the encoding cue -> openapi spec, specifically the paths parsing and encoding that was missing, I didn't plan to create the decoding stage from an openapi spec to cue, I think that's an enormous feature and, Since it's been a while I created this PR, I don't know if there's been any advances on that. @mvdan ?

@dgutson
Copy link

dgutson commented Sep 28, 2022

@araddon anything else we can help to get this in?
Could we please address openapi[yaml,json] deserialize -> non-interpreted cue -> simplified cue in another PR?
We can create an issue if necessary.

@dgutson
Copy link

dgutson commented Dec 1, 2022

ping? @araddon

@qequ
Copy link
Author

qequ commented Dec 1, 2022

ping @mvdan @rogpeppe

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

Successfully merging this pull request may close these issues.

encoding/openapi: add support for paths section
4 participants