docs: schema fixes #2375

woylie · 2022-04-07T10:55:32Z

This PR updates the API schema.

resolves #2357

turn uiNodeType into enum
turn uiNodeGroup into enum
ensure select field value for attributes.node_type matches actual node_type value (see screenshot)
add task for previewing API documentation
In api.openapi.json, the LoginFlow and the VerificationFlow list type as a required property. The other flows don't, and none of the flows in openapi.json list it. I assume the type should always be set to either "api" or "browser" and never be omitted or null. Is that so? If so, I'll add type as a required property to all the flows.

Checklist

I have read the contributing guidelines.
I have referenced an issue containing the design document if my change
introduces a new feature.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

zepatrik

Thanks for the PR. Unfortunately, this will not fix the problem as we generate the openapi schema from comments in the code. Instead, you will have to find and fix those.
Specifically you want to adjust

kratos/ui/node/attributes.go

Lines 84 to 114 in 37f2f22

    
           // ImageAttributes represents the attributes of an image node. 
        
           // 
        
           // swagger:model uiNodeImageAttributes 
        
           type ImageAttributes struct { 
        
           	// The image's source URL. 
        
           	// 
        
           	// format: uri 
        
           	// required: true 
        
           	Source string `json:"src"` 
        
           	// A unique identifier 
        
           	// 
        
           	// required: true 
        
           	Identifier string `json:"id"` 
        
           	// Width of the image 
        
           	// 
        
           	// required: true 
        
           	Width int `json:"width"` 
        
           	// Height of the image 
        
           	// 
        
           	// required: true 
        
           	Height int `json:"height"` 
        
           	// NodeType represents this node's types. It is a mirror of `node.type` and 
        
           	// is primarily used to allow compatibility with OpenAPI 3.0. 
        
           	// 
        
           	// required: true 
        
           	NodeType Type `json:"node_type"` 
        
           }

Enums can be created like this: https://github.com/ory/keto/blob/d64ae29b5aa7359792c3515a771eefc093065f9b/internal/expand/tree.go#L15-L23

So you have to adjust

kratos/ui/node/node.go

Lines 20 to 21 in 37f2f22

    
           // swagger:model uiNodeType 
        
           type Type string

to follow the structure of the other sample. It is specifically important to put the consts just below the type definition in one const group.

You can test if your changes work by running make sdk.

woylie · 2022-04-07T11:07:52Z

Thanks, I'll revise this tomorrow.

woylie · 2022-04-07T12:03:10Z

I'm sorry, I'm not very familiar with Go and I don't know what I'm doing. I tried to turn uiNodeType on the node into an enum, but the output still looks wrong.

zepatrik · 2022-04-07T12:14:40Z

Thanks, I'll revise this tomorrow.

That was a short night 😉

Swagger, the tool we use for generating the spec, is unfortunately very buggy. I think it only works if the enum name and the type name is the same. I'll quickly try that.

codecov · 2022-04-07T12:59:34Z

Codecov Report

Merging #2375 (3ed9d35) into master (ee2d410) will increase coverage by 0.00%.
The diff coverage is 92.30%.

@@           Coverage Diff           @@
##           master    #2375   +/-   ##
=======================================
  Coverage   76.61%   76.62%           
=======================================
  Files         315      315           
  Lines       17356    17358    +2     
=======================================
+ Hits        13298    13300    +2     
  Misses       3122     3122           
  Partials      936      936

Impacted Files	Coverage Δ
selfservice/flow/login/strategy.go	`80.00% <ø> (ø)`
selfservice/flow/recovery/error.go	`78.43% <ø> (ø)`
selfservice/flow/recovery/flow.go	`94.20% <ø> (ø)`
selfservice/flow/recovery/strategy.go	`70.00% <ø> (ø)`
selfservice/flow/registration/error.go	`69.84% <ø> (ø)`
selfservice/flow/registration/flow.go	`100.00% <ø> (ø)`
selfservice/flow/registration/strategy.go	`80.00% <ø> (ø)`
selfservice/flow/settings/error.go	`77.47% <ø> (ø)`
selfservice/flow/settings/flow.go	`94.02% <ø> (ø)`
selfservice/flow/settings/strategy.go	`60.00% <ø> (ø)`
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f29d45...3ed9d35. Read the comment docs.

woylie · 2022-04-07T14:21:52Z

Thanks for fixing the enums! I couldn't stop myself... I pushed some more commits.

As for the doc preview server, I didn't know why there are both api.json and swagger.json, so I added make tasks for both.

I changed the type field for all flows to be required. So far, my API client hasn't complained when I enforce this.

When I run go test -short -tags sqlite ./... on my machine, I'm getting some errors for TestWebHooks:

--- FAIL: TestWebHooks (0.54s)
    --- FAIL: TestWebHooks/uc=Pre_Registration_Hook (0.01s)
        --- FAIL: TestWebHooks/uc=Pre_Registration_Hook/auth=api_key_in_header (0.00s)
            --- FAIL: TestWebHooks/uc=Pre_Registration_Hook/auth=api_key_in_header/method=DELETE (0.00s)
                web_hook_integration_test.go:271:
                      Error Trace:  web_hook_integration_test.go:271
                      Error:        Received unexpected error:
                                    open ./stub/test_body.jsonnet: too many open files
                                    unable to fetch from source: ./stub/test_body.jsonnet
                                    github.com/ory/x/fetcher.(*Fetcher).fetchFile
                                      /Users/whatever/go/pkg/mod/github.com/ory/x@v0.0.358/fetcher/fetcher.go:87
                                    github.com/ory/x/fetcher.(*Fetcher).Fetch
                                      /Users/whatever/go/pkg/mod/github.com/ory/x@v0.0.358/fetcher/fetcher.go:58
                                    github.com/ory/kratos/request.(*Builder).readTemplate
                                      /Users/whatever/Dev/kratos/request/builder.go:197
                                    github.com/ory/kratos/request.(*Builder).addBody
                                      /Users/whatever/Dev/kratos/request/builder.go:78
                                    github.com/ory/kratos/request.(*Builder).BuildRequest
                                      /Users/whatever/Dev/kratos/request/builder.go:174
                                    github.com/ory/kratos/selfservice/hook.(*WebHook).execute
                                      /Users/whatever/Dev/kratos/selfservice/hook/web_hook.go:126
                                    github.com/ory/kratos/selfservice/hook.(*WebHook).ExecuteRegistrationPreHook
                                      /Users/whatever/Dev/kratos/selfservice/hook/web_hook.go:92
                                    github.com/ory/kratos/selfservice/hook_test.TestWebHooks.func13
                                      /Users/whatever/Dev/kratos/selfservice/hook/web_hook_integration_test.go:138
                                    github.com/ory/kratos/selfservice/hook_test.TestWebHooks.func27.9.1
                                      /Users/whatever/Dev/kratos/selfservice/hook/web_hook_integration_test.go:265
                                    testing.tRunner
                                      /nix/store/l630zyx2093kl3m425ivalj6db7a24kw-go-1.17.7/share/go/src/testing/testing.go:1259
                                    runtime.goexit
                                      /nix/store/l630zyx2093kl3m425ivalj6db7a24kw-go-1.17.7/share/go/src/runtime/asm_arm64.s:1133
                      Test:         TestWebHooks/uc=Pre_Registration_Hook/auth=api_key_in_header/method=DELETE
                web_hook_integration_test.go:273:
                      Error Trace:  web_hook_integration_test.go:273
                      Error:        Not equal:
                                    expected: "DELETE"
                                    actual  : ""

                                    Diff:
                                    --- Expected
                                    +++ Actual
                                    @@ -1 +1 @@
                                    -DELETE
                                    +
                      Test:         TestWebHooks/uc=Pre_Registration_Hook/auth=api_key_in_header/method=DELETE
                web_hook_integration_test.go:280:
                      Error Trace:  web_hook_integration_test.go:280
                      Error:        Not equal:
                                    expected: []string{"application/json"}
                                    actual  : []string(nil)

                                    Diff:
                                    --- Expected
                                    +++ Actual
                                    @@ -1,4 +1,2 @@
                                    -([]string) (len=1) {
                                    - (string) (len=16) "application/json"
                                    -}
                                    +([]string) <nil>

                      Test:         TestWebHooks/uc=Pre_Registration_Hook/auth=api_key_in_header/method=DELETE
                web_hook_integration_test.go:280:
                      Error Trace:  web_hook_integration_test.go:280
                      Error:        Not equal:
                                    expected: []string{"My-Key-Value"}
                                    actual  : []string(nil)

                                    Diff:
                                    --- Expected
                                    +++ Actual
                                    @@ -1,4 +1,2 @@
                                    -([]string) (len=1) {
                                    - (string) (len=12) "My-Key-Value"
                                    -}
                                    +([]string) <nil>

                      Test:         TestWebHooks/uc=Pre_Registration_Hook/auth=api_key_in_header/method=DELETE
                web_hook_integration_test.go:287:
                      Error Trace:  web_hook_integration_test.go:287
                      Error:        Input ('') needs to be valid json.
                                    JSON parsing error: 'unexpected end of JSON input'
                      Test:         TestWebHooks/uc=Pre_Registration_Hook/auth=api_key_in_header/method=DELETE
            --- FAIL: TestWebHooks/uc=Pre_Registration_Hook/auth=api_key_in_header/method=GET (0.00s)
panic: httptest: failed to listen on a port: listen tcp6 [::1]:0: socket: too many open files [recovered]
  panic: httptest: failed to listen on a port: listen tcp6 [::1]:0: socket: too many open files

I'm not sure what is happening.

woylie · 2022-04-07T14:30:47Z

One more question, what is the reason for all the HTML tags like <code type="make/command">make test-docs</code> in the readme, instead of just using backticks and code fences?

zepatrik · 2022-04-11T15:11:42Z

One more question, what is the reason for all the HTML tags like make test-docs in the readme, instead of just using backticks and code fences?

That's related to a tool we use for testing documentation https://github.com/kevgo/text-runner

zepatrik

Nice, looks good to me. The CI failure does not seem related, but maybe rebase onto current master and let it run again?

zepatrik · 2022-04-11T15:12:55Z

Makefile

+.PHONY: docs/api
+docs/api:
+		npx @redocly/openapi-cli preview-docs spec/api.json
+
+.PHONY: docs/swagger
+docs/swagger:
+		npx @redocly/openapi-cli preview-docs spec/swagger.json


Oh that's nice 👍

aeneasr

This is grand, thank you! Just two comments :)

ui/node/node.go

aeneasr

Awesome, thank you! 🎉 Your contribution makes Ory better :)

vinckr · 2022-04-19T08:21:51Z

Hello @woylie
Congrats on merging your first PR in Ory 🎉 !
Your contribution will soon be helping secure millions of identities around the globe 🌏.
As a small token of appreciation we send all our first time contributors a gift package to welcome them to the community.
Please drop me an email and I will forward you the form to claim your Ory swag!

Closes ory#2357 Co-authored-by: zepatrik <zepatrik@users.noreply.github.com>

woylie requested review from aeneasr and zepatrik as code owners April 7, 2022 10:55

woylie changed the title ~~Docs: schema fixes~~ docs: schema fixes Apr 7, 2022

woylie marked this pull request as draft April 7, 2022 11:01

zepatrik requested changes Apr 7, 2022

View reviewed changes

woylie requested a review from zepatrik April 8, 2022 22:04

zepatrik approved these changes Apr 11, 2022

View reviewed changes

woylie marked this pull request as ready for review April 11, 2022 21:47

woylie marked this pull request as draft April 11, 2022 21:56

aeneasr requested changes Apr 11, 2022

View reviewed changes

ui/node/node.go Outdated Show resolved Hide resolved

ui/node/node.go Outdated Show resolved Hide resolved

woylie marked this pull request as ready for review April 11, 2022 22:48

woylie requested a review from aeneasr April 14, 2022 22:50

woylie and others added 11 commits April 16, 2022 10:10

refactor: turn uiNodeType into enum

d5bc343

docs: add missing value

a44c9c8

refactor: rename Type to NodeType

d9ac27b

docs(swagger): make uiNodeType and uiNodeGroup enums

98cdff4

docs(swagger): update discriminator to match node type values

8d9e32a

chore: add makefile tasks for API documentation preview

df4acc4

docs(swagger): mark type as required for all flows

268b5d6

refactor: turn uiNodeType into enum

14c9161

refactor: rename Type to NodeType

1177d80

docs(swagger): make uiNodeType and uiNodeGroup enums

976c565

refactor: rename Type to UiNodeType

35b7d5d

refactor: rename Group to UiNodeGroup

3ed9d35

aeneasr approved these changes Apr 16, 2022

View reviewed changes

aeneasr merged commit a42a0f7 into ory:master Apr 16, 2022

woylie deleted the docs/schema-fixes branch April 17, 2022 00:10

peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this pull request Jun 30, 2023

fix(sdk): improved OpenAPI specifications for UI nodes (ory#2375)

b67af1a

Closes ory#2357 Co-authored-by: zepatrik <zepatrik@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: schema fixes #2375

docs: schema fixes #2375

woylie commented Apr 7, 2022 •

edited

Loading

zepatrik left a comment

woylie commented Apr 7, 2022

woylie commented Apr 7, 2022 •

edited

Loading

zepatrik commented Apr 7, 2022

codecov bot commented Apr 7, 2022 •

edited

Loading

woylie commented Apr 7, 2022

woylie commented Apr 7, 2022

zepatrik commented Apr 11, 2022

zepatrik left a comment

zepatrik Apr 11, 2022

aeneasr left a comment

aeneasr left a comment

vinckr commented Apr 19, 2022

	// ImageAttributes represents the attributes of an image node.
	//
	// swagger:model uiNodeImageAttributes
	type ImageAttributes struct {
	// The image's source URL.
	//
	// format: uri
	// required: true
	Source string `json:"src"`

	// A unique identifier
	//
	// required: true
	Identifier string `json:"id"`

	// Width of the image
	//
	// required: true
	Width int `json:"width"`

	// Height of the image
	//
	// required: true
	Height int `json:"height"`

	// NodeType represents this node's types. It is a mirror of `node.type` and
	// is primarily used to allow compatibility with OpenAPI 3.0.
	//
	// required: true
	NodeType Type `json:"node_type"`
	}

docs: schema fixes #2375

docs: schema fixes #2375

Conversation

woylie commented Apr 7, 2022 • edited Loading

Checklist

zepatrik left a comment

Choose a reason for hiding this comment

woylie commented Apr 7, 2022

woylie commented Apr 7, 2022 • edited Loading

zepatrik commented Apr 7, 2022

codecov bot commented Apr 7, 2022 • edited Loading

Codecov Report

woylie commented Apr 7, 2022

woylie commented Apr 7, 2022

zepatrik commented Apr 11, 2022

zepatrik left a comment

Choose a reason for hiding this comment

zepatrik Apr 11, 2022

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

vinckr commented Apr 19, 2022

woylie commented Apr 7, 2022 •

edited

Loading

woylie commented Apr 7, 2022 •

edited

Loading

codecov bot commented Apr 7, 2022 •

edited

Loading