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

WIP: Fix expand remote #137

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

Conversation

fredbi
Copy link
Member

@fredbi fredbi commented Jan 4, 2021

circular $ref expansion: fixed edge cases

  • fixes Expander: expanded document is not self-contained with some circular $ref patterns #94

  • now expanded $ref's are always contained in the resulting document.
    All circular $ref that used to resolve to a remote $ref now resolve
    as a json pointer inside the expanded document. Pointer resolution
    prefers pointers to definitions.

  • added additional test case for remote cyclical $ref, from azure API

  • schema IDs are removed from the expanded spec: schemas expanded from some schema ID reference
    now refer to the new expanded root document.

  • circular IDs are resolved against the corresponding root document.

NOTE(1): uncovered pre-existing issue with nested schema ID involving cyclical references.
This case remains unsupported and is illustrated by test case: circular_test.go#L198 ("withID")

NOTE(2): pre-existing issue with non-deterministic expansion remains unsolved,
although the election of the replacing pointer inside the root document
somewhat reduces the scope of this problem.

This case remains illustrated by a minimal test case: circular_test.go#L46 ("minimal"),
which expands correctly, but with changing results.

NOTE(3): notice that expansion is still not an idempotent transform, in the presence
of cyclical $ref's: another run on an expanded spec with remaining cyclical $ref
will expand further down and detect again the cycle.

The result remains functionally correct, as illustrated by test case: circular_test.go#L168 ("CircularID").
Notice that this test case reproduces a validation fixture from jsonschema test (passed by go-openapi/validate).

Signed-off-by: Frederic BIDON [email protected]

@codecov
Copy link

codecov bot commented Jan 4, 2021

Codecov Report

Merging #137 (e606f6b) into master (efe8fb3) will increase coverage by 0.90%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #137      +/-   ##
==========================================
+ Coverage   62.18%   63.08%   +0.90%     
==========================================
  Files          27       27              
  Lines        2052     2086      +34     
==========================================
+ Hits         1276     1316      +40     
+ Misses        605      592      -13     
- Partials      171      178       +7     
Impacted Files Coverage Δ
resolver.go 56.86% <0.00%> (ø)
expander.go 77.77% <79.16%> (-0.80%) ⬇️
schema_loader.go 89.41% <89.28%> (+1.17%) ⬆️
normalizer.go 85.71% <92.85%> (+2.13%) ⬆️
swagger.go 66.29% <0.00%> (+1.12%) ⬆️
ref.go 43.24% <0.00%> (+2.70%) ⬆️
parameter.go 22.64% <0.00%> (+3.77%) ⬆️

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 efe8fb3...e606f6b. Read the comment docs.

* fixes go-openapi#94

* now expanded $ref's are always contained in the resulting document.
  All circular $ref that used to resolve to a remote $ref now resolve
  as a json pointer inside the expanded document. Pointer resolution
  prefers pointers to definitions.

* added additional test case for remote cyclical $ref, from azure API

* schema IDs are removed from the expanded spec: schemas expanded from some schema ID reference
  now refer to the new expanded root document.

* circular IDs are resolved against the corresponding root document.

> NOTE(1): uncovered pre-existing issue with nested schema ID involving cyclical references.
> This case remains unsupported and is illustrated by test case: circular_test.go#L198 ("withID")

> NOTE(2): pre-existing issue with non-deterministic expansion remains unsolved,
> although the election of the replacing pointer inside the root document
> somewhat reduces the scope of this problem.
>
> This case remains illustrated by a minimal test case: circular_test.go#L46 ("minimal"),
> which expands correctly, but with changing results.

> NOTE(3): notice that expansion is still not an idempotent transform, in the presence
> of cyclical $ref's: another run on an expanded spec with remaining cyclical $ref
> will expand further down and detect again the cycle.
>
> The result remains functionally correct, as illustrated by test case: circular_test.go#L168 ("CircularID").
> Notice that this test case reproduces a validation fixture from jsonschema test (passed by go-openapi/validate).

Signed-off-by: Frederic BIDON <[email protected]>
@fredbi fredbi force-pushed the fix-expand-remote branch from 89c3074 to e606f6b Compare January 5, 2021 08:04
@fredbi fredbi changed the title Fix expand remote WIP: Fix expand remote Dec 5, 2023
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.

Expander: expanded document is not self-contained with some circular $ref patterns
1 participant