Skip to content

Conversation

@Wrongusername
Copy link

… case when there's placeholdered and placeholderless URIs with same method on resource

… case when there's placeholdered and placeholderless URIs with same method on resource
@Wrongusername
Copy link
Author

Note: CI seems broken / something with composer version.
Tests don't run on local / almost all broken, so can't really update them for changes anyway.

@Wrongusername
Copy link
Author

Wrongusername commented Sep 24, 2025

The problem happens when we have URIs with same HTTP method for same URI prefix with and without placeholder.

E.g.
PUT /gateways/{id}
and
PUT /gateways

THere're multiple cases of such issue at least in delivery-admin-api-client
This results in client methods getting generated with same name, and for JS client case this basically just overwrites earlier instances silently without any notification.
(For PHP likely means just crash from double defining same method).

Screenshot illustrating problem and fix
image
image

This is a breaking version change as it renames methods, so consumers must beware to properly update their code to new names, if placeholdered variant happened to come after placeholderless, as now after change placeholderless version would be calling other endpoint / previously overwritten.


How to test:
git clone https://github.com/Wrongusername/util-raml-code-generator.git
Do composer install in it.

Run command

bin/console js-generator:package <path_to_api_spec_raml_repo>/raml/app-delivery-api/admin-api-client/api.raml ./generated delivery-admin-api-client --library_name=@paysera/delivery-admin-api-client

Check ./generated/src/service/DeliveryAdminApiClient.js

Assert duplicate placeholderless/placeholdered endpoints with same URI and method are now disambiguated by by<placeHolder> suffix and no longer overwrite each other.

return '';
}

return '_by' . ucfirst(str_replace(['{', '}'], '', $part->getPlaceholder()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add test case that reflects the change, thanks. Looks like the change changes case...

Copy link
Author

Choose a reason for hiding this comment

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

@mSprunskas
I could probably add to cover specifically this case, but there's big problem with this repo and tests as noted on prev note.

Existing tests in this repo broken for me / reporting various composer package version mismatches(why are tests checking that) and at least newline mismatches(mb someone commited it with windows CRLFs) in like 98%+ of tests.

Also gitlab pipelines seem broken because of wrong composer version i guess.
I don't have scope to fix all issues of this 8-year old repo on this ticket.

So not sure if there would be much use with that state, as added test case would be only intended for manual run at that point, which is barely any use beyond what QA will check.

Copy link
Author

Choose a reason for hiding this comment

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

@Wrongusername
Copy link
Author

closed as can't run tests on this repo on my machine

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