net: http_server: allow specifying a default resource #83811
Conversation
|
Should we mention about this also in the API documentation which has examples of the usage? |
|
Added some documentation for this change |
mrodgers-witekio
force-pushed
the
http_server_default_resource
branch
from
94fb189 to
9f9b19b
Compare
|
It would be great to provide a page that can be used to communicate any HTTP status code, and Zephyr could provide one that users could choose to use. Default might not be the best terminology, but I think the idea is good. |
| @@ -50,14 +50,14 @@ static struct http_resource_detail detail[] = { | |||
| * the paths (and implementation-specific details) are known at compile time. | |||
| */ | |||
| static const uint16_t service_A_port = 4242; | |||
| HTTP_SERVICE_DEFINE(service_A, "a.service.com", &service_A_port, 4, 2, DETAIL(0)); | |||
| HTTP_SERVICE_DEFINE(service_A, "a.service.com", &service_A_port, 4, 2, NULL); | |||
There was a problem hiding this comment.
Why are any of the DETAIL() fields being changed here?
There was a problem hiding this comment.
This is primarily for the test_HTTP_RESOURCE_WILDCARD testcase. Effectively this is checking that:
- calling
get_resource_detailwith a given service and path returns the correct resource, if there is a registered resource matching the path - If there is no resource matching the given path,
get_resource_detailreturns NULL and doesn't set the path length.
By making the _detail parameter of HTTP_SERVICE_DEFINE into a default resource which is returned for any path that doesn't match another resource, get_resource_detail will never return NULL for a service which has a default detail registered.
I changed the _detail to NULL in the existing services to keep the existing behaviour of the test (ie. no resource found for non-matching paths). There is an additional service with a _detail registered to test the new behaviour of a default resource being returned if the path doesn't match anything else.
There was a problem hiding this comment.
@cfriedt any thoughts on the test suite changes?
There was a problem hiding this comment.
Sorry for the delay - I'm travelling at the moment for a work event. Looking at it right now!
There was a problem hiding this comment.
The way that detail should work is that users can pass in anything that they like that is specific to their service (in general).
It's completely arbitrary, and does not need to be a pointer to a struct http_resource_detail at all. In fact, I believe that was only done specifically for the purpose of this test, for lack of anything more imaginative. But it really should be preserved in case people need an arbitrary pointer to any kind of special context associated with their service.
For the purpose of providing a custom 404 page (or any custom error page for that matter), I would really suggest adding a function pointer to struct http_service_desc that has a similar signature to http_resource_dynamic_cb_t. It just needs to encompass whatever would be required for sending back for an http error. If the function pointer is NULL inside of the service, by all means, hard-code a default error page generator in the http library itself. If the service definition macro is able to detect that a supplied error handler is NULL, then the default one could be used, getting a reference at link time. If not, the default handler would be discarded by the linker due to having no references.
And if you wanted to, you could pass in a struct of different templates for different errors as the _detail parameter, because it's (obviously) instance-specific data. Does that make sense?
Or it could simply be NULL as well. It's really just there to be an option for the user if the user needs it for any purpose for their HTTP service.
There was a problem hiding this comment.
No worries about the delay!
Perhaps I have confused things with the idea of a custom 404 handler. My main motivation for the change is better compatibility with a single-page app using client-side routing. In this case we should respond to a request for any unknown path by serving index.html or similar (see: https://create-react-app.dev/docs/deployment/#serving-apps-with-client-side-routing , in my case I'm using preact but the concept is the same). For this I want to register a static resource as the "default", but I figured it makes sense if you can actually register any resource type (static, static FS, dynamic), and then you get the ability to have some custom 404 handling as a bonus.
This can't be reliably achieved with the existing wildcard path matching - if I register an index.html resource with a path of '/*' then the path matching becomes dependent on the order in which the linker arranges the resources in their iterable section. If you could guarantee that the '/*' path would be checked after all other resources then this would work, but if not then any resource placed after it in the linker section will never be matched. Even if the linker ordering is deterministic (e.g. alphabetical) then adding a zzz_resource somewhere could quite easily break things.
It sounds like a better approach might be to leave the _detail parameter as-is for use by the user, a bit like the user_data in the http_resource_detail_dynamic (maybe the naming and/or docs should be updated to make it clear this is what the parameter is for).
A separate 'catch-all' resource could then be added to a service via a new field, which would have the behaviour introduced in this PR - ie. get_resource_detail returns the catch-all resource if none of the resources registered in the linker section for the service are matched.
Any custom error handling should be done separately to this feature (if we're registering the catch-all resource as index.html then this is not useful for routing errors to)
There was a problem hiding this comment.
Hi @cfriedt , any thoughts on the comment above?
I think at least being able to serve index.html or similar for any unknown path would be a useful feature to get in before the 4.1.0 release. Happy to update this PR to use an extra field in the struct http_service_desc if you think the _detail field is best left for some user-defined data.
There was a problem hiding this comment.
@mrodgers-witekio - your suggestion is good from my perspective
mrodgers-witekio
force-pushed
the
http_server_default_resource
branch
from
9f9b19b to
5269975
Compare
mrodgers-witekio
force-pushed
the
http_server_default_resource
branch
from
5269975 to
46dbef8
Compare
zephyrbot
added
area: Sockets
zephyrbot
requested review from
jakub-uC,
nashif,
pdgendt,
ssharks and
tbursztyka
|
Updated PR to add an extra |
A _res_fallback parameter to HTTP_SERVICE_DEFINE is added to optionally specify a fallback resource detail, which will be served if no other resource matches the URL. Signed-off-by: Matt Rodgers <mrodgers@witekio.com>
Add documentation showing how to use the _detail parameter when registering an HTTP service to provide a default resource handling any unknown path. Also update the 4.1 release migration guide. Signed-off-by: Matt Rodgers <mrodgers@witekio.com>
mrodgers-witekio
force-pushed
the
http_server_default_resource
branch
from
46dbef8 to
e9ef1f9
Compare
Add a
_res_fallbackparameter toHTTP_SERVICE_DEFINE, which can be used to specify a fallback resource detail which will be served if no other resource is matched.This could be used to: