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

Stabilize order of parameter objects and preserve objects with newer example #59

Merged
merged 7 commits into from
Jul 8, 2022
Merged

Stabilize order of parameter objects and preserve objects with newer example #59

merged 7 commits into from
Jul 8, 2022

Conversation

exoego
Copy link
Owner

@exoego exoego commented May 27, 2022

This is a spin-out from #57
After this PR, parameters array preserves the occurrence with newer value in example

Current implementation

It keeps the first occurrence of duplicated items with same name & in.
If user update an existing parameter in their rspec, a new parameter object with newer value in example fieldis added to parameters` array ... but is removed soon because it is not the first occurrence.
In other words, the occurrence with oldest example is kept forever.

Problems

"Preserving first occurrence" will cause a problem for some cases.

1. User changed the value format.

Let's say user changed the format of id from numeric (123) to ulid (01ARZ3NDEKTSV4RRFFQ69G5FAV).

  • Base spec, [{ name: "id", in: "path", example: "123" }]
  • After merge, [{ name: "id", in: "path", example: "123" }, { name: "id", in: "path", example:"01ARZ3NDEKTSV4RRFFQ69G5FAV" }]
  • After deduplication, [{ name: "id", in: "path", example: "123" }]

😭 The preserved 123 is wrong about format.

2. rspec-openapi support another field of parameter object

Let's say rspec-openapi added description field in parameter object somehow.

  • Base spec, [{ name: "id", in: "path", example: "123" }]
  • After merge, [{ name: "id", in: "path", example: "123" }, { name: "id", in: "path", example:"123", description:"blah blah" }]
  • After deduplication, [{ name: "id", in: "path", example: "123" }]

😭 description is not added.

Solution

1. Keep a last occurrence

Unfortunately, it brings unstable order.
Let's say we invokes /foo/123 and /foo/456 in rspec.

  • 1st run
    • Base spec, [{ name: "id", in: "path", example: "123" }]
    • After merge, [{ name: "id", in: "path", example: "123" }, { name: "id", in: "path", example:"456" }]
    • After deduplication, [{ name: "id", in: "path", example: "456" }]
  • 2nd run: (just rerun)
    • Base spec, [{ name: "id", in: "path", example: "456" }]
    • After merge, [{ name: "id", in: "path", example: "456" }, { name: "id", in: "path", example: "123" } ]
    • After cleanup, [{ name: "id", in: "path", example: "123" }]

😭 123 and 456 is oscillating for each run.

2. Introduces a timestamp marker to be used as sort key

I think it is better to introduce __marker: Timestamp field to parameters.
Then, merger sorts the parameters and preserves the duplicates with largest __marker, so the older occurrencenes will be removed.

This PR also sorts parameters by in and name before __marker, because sort_by is not "stable sort"

@exoego exoego marked this pull request as draft May 27, 2022 01:58
@exoego
Copy link
Owner Author

exoego commented May 27, 2022

Hmm, all tests passed on my end 🤔

@@ -24,36 +24,36 @@ paths:
in: query
schema:
type: integer
example: 1
example: 42
Copy link
Owner Author

Choose a reason for hiding this comment

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

Note) 42 came from the updated test

@exoego exoego changed the title Preserve parameter objects with newer example Stabilize order of parameter objects and preserve objects with newer example May 27, 2022
@exoego exoego marked this pull request as ready for review May 27, 2022 13:21
@exoego
Copy link
Owner Author

exoego commented Jun 10, 2022

@k0kubun Could you take a look 🙇

@k0kubun
Copy link
Collaborator

k0kubun commented Jun 10, 2022

Sorry, I didn't read this when the CI was not passing and I didn't get a notification when it started to pass. I'll take a look.

@@ -48,13 +48,16 @@ def example_enabled?
def build_parameters(record)
parameters = []

marker_to_keep_last_duplicate = Time.now.utc.round.to_s
Copy link
Collaborator

@k0kubun k0kubun Jun 10, 2022

Choose a reason for hiding this comment

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

The PR is now concise enough to make me understand how and why it's used. Thank you.

I have some suggestions here though:

  • Can we insert them inside SchemaMerger instead of SchemaBuilder? This seems to be the concern for SchemaMerger and it feels like the current implementation scatters the implementation details to unrelated places. This PR should touch SchemaBuilder.
  • Could we achieve the same thing by just always prepending new schema elements to the array in SchemaMerger and calling uniq! after that? It should give the same result as sorting based on __marker.

Copy link
Owner Author

@exoego exoego Jul 8, 2022

Choose a reason for hiding this comment

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

Can we insert them inside SchemaMerger instead of SchemaBuilder?

It makes sense. 6326392
And it turned out that __marker is not needed. 1128a0c

Could we achieve the same thing by just always prepending new schema elements to the array in SchemaMerger and calling uniq! after that?

Yes, sort! removed 2e88f76
Without sort!, diffs on the existing rails/doc/openapi.(json|yml) is bigger.

@exoego exoego requested a review from k0kubun July 8, 2022 08:39
Copy link
Collaborator

@k0kubun k0kubun left a comment

Choose a reason for hiding this comment

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

👍

@k0kubun k0kubun merged commit ce9bb3b into exoego:master Jul 8, 2022
@exoego exoego deleted the preserver-newer-example branch July 25, 2022 06:12
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.

2 participants