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

fix(admin) proper support for PUT /{entities}/{entity}/plugins/{plugin} #4288

Merged
merged 1 commit into from
Feb 7, 2019

Conversation

thibaultcha
Copy link
Member

Prior to this patch, PUT requests to plugins under their associated
resources (e.g. /services/{service}/plugins/{plugin}) would
systematically return 404, instead of creating the plugin if it did
not exist. This was because the before Lapis hook attempts to retrieve
the plugin in the database before modifying it. Those endpoints are
preserved for backwards-compatibility purposes (being too nested to be
auto-generated), so that workaround is fine.

This patch ensures that we skip this logic with PUT requests, plus
makes sure that we set the self.args.post arguments according to the
self.params arguments (path segments with foreign UUIDs).

Thanks @gszr for the report.

@thibaultcha
Copy link
Member Author

@gszr Will you make sure this works on your side? Thanks!

@bungle
Copy link
Member

bungle commented Feb 7, 2019

. Those endpoints are preserved for backwards-compatibility purposes (being too nested to be auto-generated), so that workaround is fine

I think we might want to make this also autogenerated for all the entities. Let me explain:

  • /services/{service}/plugins/

This is the level where autogenerator stops. Now this:

  • /services/{service}/plugins/{plugin}

Is not autogenerated but extended. I think we could perhaps open that url for autogenerator just for PUT. My reason is that:

  • /services/{service}/plugins/
  • /services/{service}/plugins/{plugin}

can be considered collection endpoints when doing PUT. This makes it possible to do:
PUT /services/demo/routes/demo-route

Which is nice. I wouldn't (though I have no strong feeling about it) support entity endpoints on that path anymore (e.g. PATCH, GET), just PUT (but that is debatable). PUT would just complete the POST that we already support.

@thibaultcha
Copy link
Member Author

Not opposed to it, although:

  • We should still support PATCH and GET on those endpoints
  • We should still merge this in the meantime

…in}`

Prior to this patch, `PUT` requests to plugins under their associated
resources (e.g. `/services/{service}/plugins/{plugin}`) would
systematically return `404`, instead of creating the plugin if it did
not exist. This was because the `before` Lapis hook attempts to retrieve
the plugin in the database before modifying it. Those endpoints are
preserved for backwards-compatibility purposes (being too nested to be
auto-generated), so that workaround is fine.

This patch ensures that we skip this logic with `PUT` requests, plus
makes sure that we set the `self.args.post` arguments according to the
`self.params` arguments (path segments with foreign UUIDs).

Thanks @gszr for the report.

From #4288
@thibaultcha thibaultcha force-pushed the fix/put-admin-api-plugin branch from a1a92de to afa0f74 Compare February 7, 2019 17:12
@thibaultcha thibaultcha merged commit 6c1fb39 into master Feb 7, 2019
@thibaultcha thibaultcha deleted the fix/put-admin-api-plugin branch February 7, 2019 18:37
@hishamhm hishamhm modified the milestones: 1.0.4, 1.1.0 Feb 14, 2019
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.

3 participants