Skip to content

Fix engine path resolution - #113

Merged
exoego merged 5 commits into
exoego:masterfrom
pcoliveira:fix-engine-path-resolution
Sep 29, 2023
Merged

Fix engine path resolution#113
exoego merged 5 commits into
exoego:masterfrom
pcoliveira:fix-engine-path-resolution

Conversation

@pcoliveira

Copy link
Copy Markdown
Contributor

Closes issue #56
BREAKING CHANGE: The returned path for mounted engines now includes the path prefix.

An error was raised when testing a route that is mounted after an engine that shares the same route prefix.
The path returned for a mounted engine didn't include the route prefix (only the engine sub path was returned).

On the engine test case, the expected path was changed from /eng_route to /my_engine/eng_route, to match the mounted path.

Comment thread lib/rspec/openapi/record_builder.rb Fixed
Comment thread lib/rspec/openapi/record_builder.rb Fixed
@xcskier56

Copy link
Copy Markdown

@pcoliveira is there anything I can do to help this PR along? This has been nagging me for a long while and finally has become more of an issue

@exoego exoego added bug Something isn't working breaking-change labels May 25, 2023
end
end
raise "No route matched for #{request.request_method} #{request.path_info}"
nil

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Q) Why this error message is removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When two engines are mounted on the same root path, and the first doesn't resolve, the resolution needs to continue to the next engine. The raise was moved to the caller, when all engines are resolved

@argent-codes

Copy link
Copy Markdown

My organization recently ran into the issue fixed by this PR, so I would love to see this released ❤️

@exoego

exoego commented Aug 15, 2023

Copy link
Copy Markdown
Owner

@pcoliveira Hi. Sorry for late response.
Could you check your PR?
I left a question.

@pcoliveira

Copy link
Copy Markdown
Contributor Author

@pcoliveira Hi. Sorry for late response. Could you check your PR? I left a question.

sorry, I missed your question.

@codecov

codecov Bot commented Aug 16, 2023

Copy link
Copy Markdown

Codecov Report

Merging #113 (8456b3f) into master (6a1fbf4) will increase coverage by 0.25%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #113      +/-   ##
==========================================
+ Coverage   96.65%   96.91%   +0.25%     
==========================================
  Files          14       14              
  Lines         419      421       +2     
==========================================
+ Hits          405      408       +3     
+ Misses         14       13       -1     
Files Changed Coverage Δ
lib/rspec/openapi/record_builder.rb 100.00% <100.00%> (+1.53%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pcoliveira

Copy link
Copy Markdown
Contributor Author

@exoego How may I help? is there anything I need to do to move the PR forward?

@exoego exoego left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

LGTM
This is amazing.
Sorry for the late response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants