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

OpenAPI 3.0.3 and 3.1 full support #1136

Merged
merged 314 commits into from
Apr 14, 2024
Merged

Conversation

mdaneri
Copy link
Contributor

@mdaneri mdaneri commented Aug 31, 2023

Description of the Change

Revamped OpenAPI implementation

New Features

Sample

Documentation

The following new functions have been added:

New CmdLets

  • OpenAPI
    • Test-PodeOAJsonSchemaCompliance
    • Test-PodeOADefinition
    • Test-PodeOADefinitionTag
    • Select-PodeOADefinition
    • Add-PodeOAInfo
    • Add-PodeOAExternalDoc
    • New-PodeOAExternalDoc
    • Add-PodeOATag
    • Merge-PodeOAProperty
    • Add-PodeOAServerEndpoint
    • New-PodeOAExample
    • New-PodeOAEncodingObject
    • New-PodeOAResponse
    • Add-PodeOACallBack
    • New-PodeOAResponseLink
    • New-PodeOAContentMediaType
    • New-PodeOAMultiTypeProperty
    • Add-PodeOAExternalRoute
    • New-PodeOAServerEndpoint
    • Add-PodeOAWebhook
  • OpenAPI Components
    • Add-PodeComponentGroup
    • Add-PodeOAComponentHeader
    • Add-PodeOAComponentExample
    • Add-PodeOAComponentParameter
    • Add-PodeOAComponentResponseLink
    • Add-PodeOAComponentCallBack
    • Add-PodeOAComponentPathItem
    • Add-PodeOAWebhook
    • Test-PodeOAComponent
  • OpenAPI Definitions
    • Test-PodeOADefinition
    • Select-PodeOADefinition
  • Response Helper
    • Write-PodeYamlResponse
  • Utility
    • ConvertFrom-PodeXML

Related Issue

@mdaneri
Copy link
Contributor Author

mdaneri commented Aug 31, 2023

new cmd-lets
'Test-PodeOARequestSchema',
'New-PodeOAExtraInfo',
'Add-PodeOAExternalDoc',
'Add-PodeOATag',
'Add-PodeOAComponentHeaderSchema'

Copy link
Owner

@Badgerati Badgerati left a comment

Choose a reason for hiding this comment

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

I've managed to review most of the work, but still need to go through the rest. Some very good work here! 😊

One thing I'll point out is the styleguide from the contributing doc here: https://github.com/Badgerati/Pode/blob/develop/.github/CONTRIBUTING.md#code - namely the bracers part 😅 On if, for, and switch cases for example the first brace throughout Pode is on the same line as the statement - something to consider flipping to reduce the lines changed count 😄

src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
@mdaneri
Copy link
Contributor Author

mdaneri commented Sep 11, 2023

My visual studio code is doing it automatically. I tried to change the editor setting without luck.
What editor are you using?

@Badgerati
Copy link
Owner

Badgerati commented Oct 8, 2023

Hi @mdaneri,

The editor I'm using is VS Code.

Just to let you know, I've not forgotten about this PR! I've been doing some work around Authentication which I'm just put some finishing touches to - the changes do slightly touch the OpenAPI files but only for minor tweaks, I'm not expecting them to cause any major issues here (famous last words!), but some files could conflict.

@mdaneri
Copy link
Contributor Author

mdaneri commented Oct 8, 2023

Please submit your changes I can merge them on my side

@Badgerati
Copy link
Owner

To help with the formatting, and to reduce the changes in files (going through just Helpers.ps1 alone is giving me a headache with all the { changes 😂), I've opened #1169 to enforce workspace PowerShell code formatting in VSCode - and to reformat the main /src files too.

Because of the way the auto-formatting works, it will actually more closely match the auto-formatting that's occurring on your side, with bracers on the same line as functions, and padding out hashtables, etc.

I've been meaning to do this for a while to help others as well. I'll aim to do this tomorrow.

@Badgerati
Copy link
Owner

Hi @mdaneri,

I've merged #1169. Something I just spotted is that you've added .vscode/settings.json to the .gitignore, this'll need removing as the auto code-formatting is done using this file. Might be worth reviewing what changes you have in this file to make sure they don't conflict with Pode's now defined workspace settings.

@mdaneri
Copy link
Contributor Author

mdaneri commented Oct 17, 2023 via email

Copy link
Owner

@Badgerati Badgerati left a comment

Choose a reason for hiding this comment

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

I've reviewed all the non-OpenAPI files, of which I'll shall attempt to do tomorrow/Friday. I have to say I love the "Bookmarks" page idea!

For everything YAML, I'm going to say it's best to remove it from this PR and we'll deal with it separately in #1164 (and #1165). There is the Pode.YAML module, and if we're saying (which I kind of agree with) to pull YAML support directly into Pode, I would rather do it using the powershell-yaml module - this way we don't have to roll a custom YAML converter/parser which will have to be maintained on top of Pode.

src/Misc/default-redoc.html.pode Outdated Show resolved Hide resolved
src/Misc/default-rapipdf.html.pode Outdated Show resolved Hide resolved
src/Misc/default-rapipdf.html.pode Outdated Show resolved Hide resolved
src/Misc/default-rapipdf.html.pode Outdated Show resolved Hide resolved
src/Pode.psd1 Outdated Show resolved Hide resolved
src/Private/Helpers.ps1 Outdated Show resolved Hide resolved
src/Public/Responses.ps1 Outdated Show resolved Hide resolved
@mdaneri
Copy link
Contributor Author

mdaneri commented Oct 18, 2023

I had some issues with powershell-yaml on Linux. I was thinking of making a fork of the project, making it more Linux-friendly, and updating the .NET library to v6 and v7.
But in the end, it was much easier to create a new cmdlet using an old sample.
That is the reason I created one ad-hoc. To support OpenApi, what I wrote is good enough and simple to maintain.
Ultimately, you are dealing only with arrays/hashtables of string, number, and boolean. There is no complex type we have to deal with.

I can try to work with the Pode YAML module again, but you have to publish a working version. The one that is available on Powershell Gallery needs to be fixed. It's complaining regarding a -now parameter that doesn't exist.

@mdaneri
Copy link
Contributor Author

mdaneri commented Oct 19, 2023

I just tried to use powershell-yaml
and this is the error Get-Serializer: Unable to find type [StringQuotingEmitter].

I modified helper.ps1 ConvertTo-PodeYamlInternal to use powershell-yaml

function ConvertTo-PodeYamlInternal {

    [OutputType('System.String')]

    [CmdletBinding()]
    param (
        [parameter(Position = 0, Mandatory = $true, ValueFromPipeline = $true)]
        [AllowNull()]
        $InputObject,
        [parameter(Position = 1, Mandatory = $false, ValueFromPipeline = $false)]
        [int]$Depth = 20,
        [parameter(Position = 2, Mandatory = $false, ValueFromPipeline = $false)]
        [int]$NestingLevel = 0,
        [parameter(Position = 3, Mandatory = $false, ValueFromPipeline = $false)]
        [int]$XMLAsInnerXML = 0,
        [parameter(Position = 4, Mandatory = $false, ValueFromPipeline = $false)]
        [switch] $NoNewLine
    )
    if ((Test-PodeModuleInstalled -Name 'powershell-yaml')) {
        $res = ConvertTo-Yaml -Data $InputObject
        return $res
    }

Now I'm trying with PSYaml

@mdaneri
Copy link
Contributor Author

mdaneri commented Oct 19, 2023

PSYaml works fine
I'm modifying the code to check if PSYaml is available will be used instead of the internal converter.

@Badgerati
Copy link
Owner

Which version of powershell-yaml were you using? According to cloudbase/powershell-yaml#97 that serializer error should be fixed in 0.4.7 🤔

Agreed that Pode.Yaml needs fixing, I have also considered bringing the YAML functionality directly into Pode. If you want to make this logic work with PSYaml, I'll go through and port over I'll the YAML logic from Pode.Yaml once merged - with some extra logic/flags.

@mdaneri
Copy link
Contributor Author

mdaneri commented Oct 21, 2023

Good question I'm using the one available on the PowerShell gallery.
Anyway, have you checked my last changes regarding Yaml support?
Practically, I removed the public coverto-PodeYaml and included the logic to use a different yaml library if available.
By doing that, I can generate the open API YAML version, but no one externally can use it.

@mdaneri
Copy link
Contributor Author

mdaneri commented Oct 21, 2023

Another thing I have done lately is to enable the Open API properties definition to be piped. This makes the code much cleaner and more powershell.
I still have to add the logic for objects. Because an object can be a property or a properties container, and so far, I can only pipe an object as simple property

Copy link
Owner

@Badgerati Badgerati left a comment

Choose a reason for hiding this comment

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

I've gotten about 30-40% of the way through Public/OpenAPI - still yet to do Private/OpenAPI.

I don't see the YAML changes you were mentioning, so I'm assuming a rogue missing commit? I'd also recommend testing in PS5.1 😜

src/Public/Responses.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
@Badgerati Badgerati added this to the 2.10.0 milestone Oct 21, 2023
@mdaneri
Copy link
Contributor Author

mdaneri commented Oct 21, 2023

I’m going to try with 5.1. I forgot that Microsoft still supports it 😊
The Yaml is not on the OpenApi file but on private/helper.ps1

@mdaneri
Copy link
Contributor Author

mdaneri commented Oct 22, 2023

I think to have addressed all your concerns.
let me know

src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
@mdaneri
Copy link
Contributor Author

mdaneri commented Oct 22, 2023

I have a question regarding my last improvement New-PodeOA......Property and the pipeline.

 New-PodeOAIntProperty -Name 'id'-Format Int64 -ReadOnly -Example 10 |
            New-PodeOAIntProperty -Name 'petId' -Format Int64 -Example 198772 | 
            New-PodeOASchemaProperty -Name 'Address' -ComponentSchema 'Address' |
            New-PodeOAObjectProperty -Name 'Order' -Xml @{'name' = 'order' } -PropertiesFromPipeline7 |
            New-PodeOAStringProperty -Name 'shipDate' -Format Date-Time |
            New-PodeOAStringProperty -Name 'status' -Description 'Order Status' -Example 'approved' -Enum @('placed', 'approved', 'delivered') |
            New-PodeOABoolProperty -Name 'complete' |
            New-PodeOASchemaProperty -Name 'Address' -ComponentSchema 'Address' |
            New-PodeOAObjectProperty -Name 'Order' -Xml @{'name' = 'order' } -PropertiesFromPipeline

this creates an Object from the pipeline.
still there is the old -properties parameter.

The question is regarding -PropertiesFromPipeline switch. Do you think this should be the default behavior?
I mean by default New-PodeOAObjectProperty behave like any other PodeOAProperty and can be concatenated.

like on this example:

New-PodeOAStringProperty -Name 'lastName' -Example 'James' | 
 New-PodeOAObjectProperty -Name 'User' -Xml @{'name' = 'user' } -Properties  (
            New-PodeOAIntProperty -Name 'id'-Format Int64 -Example 1 -ReadOnly |
                New-PodeOAStringProperty -Name 'username' -Example 'theUser' -Required )| 
                  New-PodeOAStringProperty -Name 'username' -Example 'theUser' -Required |
                New-PodeOAStringProperty -Name 'firstName' -Example 'John' |
                New-PodeOAStringProperty -Name 'lastName' -Example 'James' | 
                New-PodeOAObjectProperty -Name 'test' -PropertiesFromPipeline

What do you think ?

@mdaneri
Copy link
Contributor Author

mdaneri commented Oct 22, 2023

regarding the test-json and 5.1 compatibility. I'm going to put a version check and if PowerShell is 5.1 return true without doing anything.

Still, I've to work on the Test-PodeOARequestSchema because at the moment oneOf, allOf and anyOff are not supported.

src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Private/Helpers.ps1 Outdated Show resolved Hide resolved
src/Private/Helpers.ps1 Outdated Show resolved Hide resolved
src/Public/Responses.ps1 Show resolved Hide resolved
src/Private/Helpers.ps1 Outdated Show resolved Hide resolved
src/Public/Responses.ps1 Outdated Show resolved Hide resolved
@Badgerati
Copy link
Owner

@mdaneri, that should be everything currently committed reviewed! :)

Something I spotted just was the docs haven't bee updated currently - are you just waiting until you've got the work done?

@Badgerati
Copy link
Owner

I have a question regarding my last improvement New-PodeOA......Property and the pipeline.

 New-PodeOAIntProperty -Name 'id'-Format Int64 -ReadOnly -Example 10 |
            New-PodeOAIntProperty -Name 'petId' -Format Int64 -Example 198772 | 
            New-PodeOASchemaProperty -Name 'Address' -ComponentSchema 'Address' |
            New-PodeOAObjectProperty -Name 'Order' -Xml @{'name' = 'order' } -PropertiesFromPipeline7 |
            New-PodeOAStringProperty -Name 'shipDate' -Format Date-Time |
            New-PodeOAStringProperty -Name 'status' -Description 'Order Status' -Example 'approved' -Enum @('placed', 'approved', 'delivered') |
            New-PodeOABoolProperty -Name 'complete' |
            New-PodeOASchemaProperty -Name 'Address' -ComponentSchema 'Address' |
            New-PodeOAObjectProperty -Name 'Order' -Xml @{'name' = 'order' } -PropertiesFromPipeline

this creates an Object from the pipeline. still there is the old -properties parameter.

The question is regarding -PropertiesFromPipeline switch. Do you think this should be the default behavior? I mean by default New-PodeOAObjectProperty behave like any other PodeOAProperty and can be concatenated.

like on this example:

New-PodeOAStringProperty -Name 'lastName' -Example 'James' | 
 New-PodeOAObjectProperty -Name 'User' -Xml @{'name' = 'user' } -Properties  (
            New-PodeOAIntProperty -Name 'id'-Format Int64 -Example 1 -ReadOnly |
                New-PodeOAStringProperty -Name 'username' -Example 'theUser' -Required )| 
                  New-PodeOAStringProperty -Name 'username' -Example 'theUser' -Required |
                New-PodeOAStringProperty -Name 'firstName' -Example 'John' |
                New-PodeOAStringProperty -Name 'lastName' -Example 'James' | 
                New-PodeOAObjectProperty -Name 'test' -PropertiesFromPipeline

What do you think ?

If I understand what you're asking, then because the -Properties currently come from the parameter itself, the -PropertiesFromPipeline switch is the better approach. Otherwise it'd be a breaking change for people to have to re-jig their code to support the new format.

Ooorr, if -Properties is null/empty, then default to from the pipeline instead - that could work 🤔

src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Private/OpenApi.ps1 Outdated Show resolved Hide resolved
@mdaneri
Copy link
Contributor Author

mdaneri commented Apr 4, 2024

No dependency

@Badgerati
Copy link
Owner

The changes being made in #1276 are in this PR, similar to the others I'll review/merge that one first then come back here, just so it's simpler :)

@Badgerati
Copy link
Owner

Hey @mdaneri, this just needs a rebase with develop and some conflicts resolving, then I can review :)

@mdaneri
Copy link
Contributor Author

mdaneri commented Apr 8, 2024

Sorry, today I couldn't.
I was traveling, and with an iPad, it was impossible to solve conflicts. Or at least I’ve no idea how to do it.
I'll try tonight

@mdaneri
Copy link
Contributor Author

mdaneri commented Apr 8, 2024

@Badgerati done

Copy link
Owner

@Badgerati Badgerati left a comment

Choose a reason for hiding this comment

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

I've been through all the code/docs again - mostly just minor things here and there. It's getting late here, but I'll run a few tests/examples tomorrow and see if there's anything to raise from there as well

docs/Tutorials/Routes/Examples/WebPages.md Outdated Show resolved Hide resolved
src/Misc/default-swagger-editor.html.pode Outdated Show resolved Hide resolved
src/Misc/default-doc-bookmarks.html.pode Outdated Show resolved Hide resolved
examples/web-rest-openapi.ps1 Outdated Show resolved Hide resolved
examples/web-rest-openapi-simple.ps1 Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Show resolved Hide resolved
src/Public/OpenApi.ps1 Show resolved Hide resolved
docs/Tutorials/OpenAPI/OpenAPI.md Outdated Show resolved Hide resolved
docs/Tutorials/OpenAPI/OpenAPI.md Outdated Show resolved Hide resolved
docs/Tutorials/OpenAPI/OpenAPI.md Outdated Show resolved Hide resolved
src/Public/OpenApi.ps1 Outdated Show resolved Hide resolved
src/Private/Helpers.ps1 Outdated Show resolved Hide resolved
src/Private/Helpers.ps1 Outdated Show resolved Hide resolved
src/Private/Helpers.ps1 Outdated Show resolved Hide resolved
src/Private/Helpers.ps1 Outdated Show resolved Hide resolved
Copy link
Owner

@Badgerati Badgerati left a comment

Choose a reason for hiding this comment

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

That's it, all looks good! 🥳 🎉 If you're happy that everything you want is here, I'll look at merging 😄

@mdaneri
Copy link
Contributor Author

mdaneri commented Apr 14, 2024

I’m good but please squash and merge. There are too many commits on this branch😊

@Badgerati Badgerati merged commit 5498974 into Badgerati:develop Apr 14, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants