Consolidate common API and UI functions #156
Conversation
|
One limitation of this is that the input validation logic remains duplicated across the I'm happy to explore this as part of this PR, or in a subsequent PR - it's highly desirable but I wanted to get feedback before I put time into it. |
|
I totally agree with trying to remove any duplication , and , I think if its not done now/soon then it will become harder to do. I also think that it makes sense to resolve the issues with View Models and Messages, but I think that should be done in a separate PR as this one is already quite big. One thing I have been wondering about is should we implement just APIs and then use a SPA approach for the web UI? (Blazor????? I know there are challenges here but we can only expect it to get better and it does in dotnet 6) I know this is a big change but again its one of those that we really should do sooner rather than later. |
|
|
||
| namespace Hippo.ControllerCore | ||
| { | ||
| public abstract class ApplicationControllerCore : HippoController |
There was a problem hiding this comment.
Can we find a different name for this (HippoCoreController?) its a bit confusing using Application.
There was a problem hiding this comment.
This is not intended to become a generic base controller though (HippoController is that). This should end up with application-related functions only.
I should have said more about where I see this going:
- Eventual full coverage of UI functions in the API
- "Controller cores" for applications, channels, revisions, etc. each containing the shared business logic for the API and UI
- UI and API controllers inheriting their respective cores but surfacing them in different ways
Ha ha, that's pretty much what I did, I just did the Create Channel core at first, but then couldn't stop myself doing Create Application as well because it seemed so tiny. Should have listened to my instincts. I can certainly back out the last commit ("Consolidate app creation") if you would like! |
|
@simongdavies Are your comments (specifically the one about backing out the final Create Application commit) blocking? I'd like to merge at least the channel stuff before I incur some sort of ghastly and motivation-sapping rebase. |
| if (request.RevisionSelectionStrategy == ChannelRevisionSelectionStrategy.UseSpecifiedRevision && revision == null) | ||
| { | ||
| LogIfNotFound(revision, request.RevisionNumber); | ||
| // TODO: not sure if this is the right response |
There was a problem hiding this comment.
If a "create channel" request comes in and the revision does not exist, the server understands the content type, and the request was well-formed, but it cannot handle the request. In this case, a 422 Unprocessable Entity with an appropriate error message ("revision '0.1.0' does not exist) is the correct response.
See also https://datatracker.ietf.org/doc/html/rfc4918#section-11.2
There was a problem hiding this comment.
422 is WebDAV. https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#client_error_responses
I agree that 404 is not right here (because the address was found; what was not found was a resource referenced in the request). Maybe it is 409, or the all-encompassing 400.
There was a problem hiding this comment.
I mean we could just create the channel and it would just start out in an unhealthy state. Maybe that's the right thing to do. We certainly let it happen with rules-based channels. But it feels surprising when the user is being specific.
There was a problem hiding this comment.
Well that sucks.
422 Unprocessable Entity describes our exact predicament. And I don't think a 400 Bad Request is a perfect fit here, either.
From RFC 4918 (emphasis in bold):
The 422 (Unprocessable Entity) status code means the server
understands the content type of the request entity (hence a
415(Unsupported Media Type) status code is inappropriate), and the
syntax of the request entity is correct (thus a 400 (Bad Request)
status code is inappropriate) but was unable to process the contained
instructions. For example, this error condition may occur if an XML
request body contains well-formed (i.e., syntactically correct), but
semantically erroneous, XML instructions.
Reading RFC4918... It doesn't seem like code 422 is meant to be used elsewhere other than with webDAV :<
https://datatracker.ietf.org/doc/html/rfc4918#section-1
While the status codes provided by HTTP/1.1 are sufficient to
describe most error conditions encountered by WebDAV methods, there
are some errors that do not fall neatly into the existing categories.
This specification defines extra status codes developed for WebDAV
methods (Section 11) and describes existing HTTP status codes
(Section 12) as used in WebDAV. Since some WebDAV methods may
operate over many resources, the Multi-Status response (Section 13)
has been introduced to return status information for multiple
resources. Finally, this version of WebDAV introduces precondition
and postcondition (Section 16) XML elements in error response bodies.
And code 422 falls under section 11, sadly.
I mean we could just create the channel and it would just start out in an unhealthy state. Maybe that's the right thing to do. We certainly let it happen with rules-based channels. But it feels surprising when the user is being specific.
I agree. I think that would be unintended behaviour from the viewpoint of a CLI. Makes sense from a web form with user interaction, but I don't think that will fly in a CI/CD scenario.
There was a problem hiding this comment.
That being said, given the choice between using an error code that doesn't explicitly cover the situation vs. one from an HTTP/1.1 extension RFC that describes the situation exactly, I would choose the latter.
There was a problem hiding this comment.
Oh! One last bit of evidence: Section 21 states that the status codes introduced in thie RFC (including 422 Unprocessable Entity) are to be updated in the IANA registry. And in the registry there is no limitation on the status code being webDAV-specific - just that it links back to RFC4918. It's grouped under the generic 4xx client error status code section.
All in all I think it's safe to use 422 Unprocessable Entity in non-webDAV scenarios. The RFC doesn't seem to be explicitly clear about this, but also see my earlier argument above.
There was a problem hiding this comment.
Yeah good point about 400. I still feel 409 might be closer to where we are: "a request conflicts with the current state of the server." And this is very much about the current state...
There was a problem hiding this comment.
After derailing standup to have everyone bicker about this wider consultation we are going to go with 400 as the generic "error we don't know how to express with a code." #StillTeam409Dammit
There was a problem hiding this comment.
Okay. It sounds like we're in agreement that a 404 is the incorrect response. But the ambiguity of each other status code means we can make a valid argument both for and against codes 400, 409, and 422. So really any of those three will work.
The reality is that we're splitting hairs here... Most users won't care what the response code is - only the error message from the CLI.
I leave the decision to you. :)
There was a problem hiding this comment.
This happens EVERY DAMN TIME with APIs that involve validation. Endless parsing of the spec, deep reading of the runes to try to figure out what "validation error" maps to. Is it 400, is it 403, is it 409, blah blah blah. 422 is a refreshing addition but WHY DON'T THEY JUST MAKE A STATUS CODE FOR VALIDATION FAILED WHY.
| if (application == null) | ||
| { | ||
| LogIfNotFound(application, request.ApplicationId); | ||
| return NotFound(); |
There was a problem hiding this comment.
If a "create channel" request comes in and the application does not exist, the server understands the content type, and the request was well-formed, but it cannot handle the request. In this case, a 422 Unprocessable Entity with an appropriate error message ("application ID 'foo' does not exist) is the correct response.
See also https://datatracker.ietf.org/doc/html/rfc4918#section-11.2
We have two functions at the moment that are available through both the API and UI. I would like to improve error reporting on these, but first I would like to deduplicate the code so I can do it in one place and be confident it works in both. This PR therefore introduces a shared controller layer which can be used from either API or UI, and delegates the interesting and non-message-specific bits of the logic to that layer.
At some point, this common layer should split out to separate application and channel controllers, as already happens in the API controller layer. However, the UI layer currently has a monolithic application controller so we can't do that yet.