feat(admin) targets can be deleted by their ID #2304
Conversation
subnetmarco
force-pushed
the
feat/delete-target-by-id
branch
from
cdb4de3 to
4eb9b7c
Compare
kong/api/crud_helpers.lua
Outdated
| return helpers.yield_error(err) | ||
| end | ||
|
|
||
| -- We know username and id are unique, so if we have a row, it must be the only one |
subnetmarco
force-pushed
the
feat/delete-target-by-id
branch
from
fe03552 to
e05f20f
Compare
|
Updated. |
| end, | ||
|
|
||
| DELETE = function(self, dao_factory) | ||
| clean_history(self.upstream.id, dao_factory) | ||
|
|
||
| -- this is just a wrapper around POSTing a new target with weight=0 | ||
| local data, err = dao_factory.targets:insert({ | ||
| target = self.params.target, | ||
| target = self.target.target, |
There was a problem hiding this comment.
The target field name is confusing with the target entity, should be really called address ("Targets can have an address and a weight" as opposed to "Targets can have a target and a weight")
There was a problem hiding this comment.
We have a target object and a target field. Sounds like we need a breaking API change then?
There was a problem hiding this comment.
(also out of scope for the PR ;) )
There was a problem hiding this comment.
Not in this PR of course - just a consideration that it's a confusing naming.
kong/api/crud_helpers.lua
Outdated
| return helpers.yield_error(err) | ||
| end | ||
|
|
||
| -- We know target and id are unique, so if we have a row, it must be the only one |
There was a problem hiding this comment.
overly pedantic, nonblocking note on this comment "we know target and id are unique" - to what does id refer to here? the id of the target? the upstream?
also, certainly target is not unique in this case. :)
There was a problem hiding this comment.
should be we can have multiple targets here, but anyone will do as they all have the same 'target' field, so just pick the first
There was a problem hiding this comment.
Which makes me wonder, should it be unique? Currently I can have three times a Target with the same target and weight > 0, but does it make sense?
Should we check that every time you add a Target, if a Target with weight > 0 and same target already exists, then return 409 Conflict?
There was a problem hiding this comment.
no, the new one overrides the old one, but the old one must be preserved in the history
There was a problem hiding this comment.
Problem is, calling /targets/active/ still shows all of them, not just the last one, which doesn't make it clear if all of them are active or just the last one.
There was a problem hiding this comment.
that does not matter, because all of those records might differ on the weight property, but they will all have the same target field, and thats the field you're looking for.
There was a problem hiding this comment.
both of these issues addressed with PRs- we still need to resolve the erroneous comment :)
Full changelog
DELETEmethod for upstream targets, allowing them to be deleted byidas well.