Skip to content

WIP: Fix expand remote #137

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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 fredbi@yahoo.com

@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 <fredbi@yahoo.com>
@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
# for free to join this conversation on GitHub. Already have an account? # 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