HTTP API cleanup #4272
HTTP API cleanup #4272
Conversation
Clarify what this function does: it implements the legacy HTTP API parser.
Rather add this token in to all call sites, check for it only where it matters: when we've got an HTTP request. If it's passed in other contexts, it will be safely ignored.
Internal requests are indicated by passing nullptr for the request argument. If we *are* in fact called from an HTTP request, we must generate some kind of reply anyways, so this parameter is obsolete.
No need to even consider this for non-web requests. Move the request special case to that context as well.
| @@ -482,7 +482,17 @@ void initServer() | |||
| return; | |||
| } | |||
|
|
|||
| if(handleSet(request, request->url())) return; | |||
| if (request->url().indexOf("win") == 0) { | |||
| if (handleHttpApi(request->url().substring(3))) { | |||
There was a problem hiding this comment.
Is it 3 or is it 4?
I would think 4 as you do not compare & in handleHttpApi() or there is no need to add equality to comparisons there.
There was a problem hiding this comment.
The leading '&' is intentionally left on as some of the test strings in handleHttpApi() have a leading "&", eg. pos = req.indexOf(F("&S="));. I'm not really sure why some do and some don't -- I should probably just clean that up too while I'm in there.
There was a problem hiding this comment.
Actually, given that, I'm going to convert this to draft for now and do some more testing. I think there's some corner cases where this code will differ from the original.
| if(handleSet(request, request->url())) return; | ||
| if (request->url().indexOf("win") == 0) { | ||
| if (handleHttpApi(request->url().substring(3))) { | ||
| if (request->url().indexOf(F("&FX=")) > 0) { |
There was a problem hiding this comment.
Wouldn't it be better to move this into handleHttpApi()?
IMO it's place is there.
There was a problem hiding this comment.
In the original code, we call unloadPlaylist() only the FX was changed from an HTTP request, but not from any other context. It made more sense to me to place that code only in the HTTP request context, rather than add a special parameter to handleHttpApi() just to switch this call on or off. It does mean naming the tag twice, but I think that's the lesser evil -- https://sandimetz.com/blog/2016/1/20/the-wrong-abstraction
An alternative might be to augment the return code to indicate if the FX value was changed, rather than parsing the tag multiple times.
There was a problem hiding this comment.
IMO any change to FX should unload playlist - unless loaded from playlist itself.
There was a problem hiding this comment.
I agree -- although this PR was aimed to be a refactor with no behavioural changes, and the original logic only unloaded the playlist in the context of an HTTP request.
I'll do a more thorough review of the other call contexts and see if I can draw a better map. No reason not to further improve things as we go.
Along with #4271, I took the opportunity to clean up the surrounding code a small amount. This PR:
nullptrfor therequest. The conditional was broken anyhow, and HTTP requests are required to generate a response.