api: check unit name on set operations #869
Conversation
|
fixes #862 |
| su.Name = item | ||
| } | ||
| if item != su.Name { | ||
| sendError(rw, http.StatusNotAcceptable, fmt.Errorf("item name %q differs to given unit name %q", item, su.Name)) |
There was a problem hiding this comment.
I guess these could do with tests. Maybe in the morning.
There was a problem hiding this comment.
Yes, and how about we clarify what "item name" means since it isn't exactly clear to an end-user. How about this: "name in URL %q differs from name in request body %q"
|
LGTM after error string and testing is addressed |
|
Updated. One thing: I'm not entirely convinced either way about status codes (specifically
.. but Any strong feelings? |
|
They should both be BadRequest |
|
"both" as in the mismatch and What about |
|
Doing what I think is best. Changed them all. |
| @@ -74,11 +85,11 @@ func (ur *unitsResource) set(rw http.ResponseWriter, req *http.Request, item str | |||
| if eu == nil { | |||
| if len(su.Options) == 0 { | |||
| err := errors.New("unit does not exist and options field empty") | |||
| sendError(rw, http.StatusConflict, err) | |||
| sendError(rw, http.StatusBadRequest, err) | |||
There was a problem hiding this comment.
BadRequest is effectively a syntax error, while Conflict means "I can't do that right now due to some other conditions that you may be able to resolve". In this case, if the unit already existed, the request would have succeeded.
|
please give me a ship it, please please |
|
🍧 with squash |
|
yum! |
jonboulle
force-pushed
the
862_valid_names
branch
from
5b22f71
to
5ef5419
Compare
api: check unit name on set operations
since we essentially have two fields where the unit name can be set I think it's safest to take this path and ensure they're the same. I also don't feel strongly about the auto-fill so can remove that if you don't like it.
validateName is dumb right now but anticipating #867....