Define the basic server types and interfaces and add a minimal example. #21

jbearer · 2022-06-15T23:16:35Z

Closes #13
Closes #14

Closes #13 Closes #14

pictographer

Looks great. I'm very grateful. All my comments are about understanding, not change requests.

pictographer · 2022-06-16T18:23:28Z

src/route.rs

+    body: T,
+) -> Result<tide::Response, RouteError<E>> {
+    let ty = best_response_type(accept, &[mime::JSON, mime::BYTE_STREAM])?;
+    if ty == mime::BYTE_STREAM {


Is there some reason to prefer cascading if/else to a match expression here?

This is copied from net which is very old. But IIRC I had to do it like this way because the constants in mime, like mime::BYTE_STREAM, aren't compile time constants so they can't be used in a match.

pictographer · 2022-06-16T18:29:22Z

src/app.rs

+    {
+        match self.apis.entry(base_url.to_string()) {
+            Entry::Occupied(_) => {
+                return Err(AppError::ModuleAlreadyExists);


This is probably what we want for now. I could imagine other applications replacing modules at runtime to make it easier to reason about modal behavior such as responses when

Authorized vs not authorized

While performing a long data structure rebuild

Progressive revelation, i.e. certain routes only become available after some proficiency is demonstrated

Features have been paid for
None of these are use cases we care about, but if we want tide-disco to get traction, could consider as an enhancement.

I think a lot of those use cases are better handled with internal mutability in the state, rather than replacing the module itself. But point taken, replacing a module is something we could consider in the future (another use case that comes to mind is hot loading modules for upgrades without down time)

pictographer · 2022-06-16T18:31:28Z

src/request.rs

+}
+
+#[derive(Clone, Debug)]
+pub enum RequestParamValue {


There's a nearly identical structure called UrlSegment in lib.rs. Do we need two? UrlSegment has each of the fields wrapped in an Option so it can work for RequestParamType as well. Less repetition, but not as clean?

Mostly I just wanted to move it into this module to avoid a circular dependency. But also, I'm not crazy about the Option thing. While it does get rid of some code, it also introduces code that needs to either unwrap or handle a None value when in reality the value can't ever be None

pictographer · 2022-06-16T18:35:03Z

src/route.rs

+) -> Result<Mime, RouteError<E>> {
+    match accept {
+        Some(accept) => {
+            // The Accept type has a `negotiate` method, but it doesn't properly handle


Is this a Tide bug? If so, we should either drop the Issue link here or file the issue and then drop the link here.

Its an http_types bug (which tide depends on). I can't remember if there was ever an issue for this, I'll go hunting and file one if I can't find anything

http-rs/http-types#402

pictographer · 2022-06-16T18:54:54Z

src/app.rs

+    /// Register an API module.
+    pub fn register_module<ModuleError>(
+        &mut self,
+        base_url: &str,


Is there an easy way to combine two sets of routes that should both have the same base_url? I.e. routes like /version, /healthcheck, and /help should be at the top level, but they also should come from a standard reusable module. If these are at the top level does that mean all the application stuff needs to have a base_url like "https://acme.com/appname/"?

I think there's a few questions here, so here are a few answers:

currently there is no way to combine two sets of routes under the same base_url. It is assumed that each module you register has its own base url. This could be relaxed if we want to

special top level routes like /version, /healthceck, and /help are handled specially and indeed registered with no base url (automatically, internally to this library). I'm going to make a separate PR using /healthcheck as an example, and we can discuss it more there

it should be possible to register a user-defined module at the top level, which is especially useful if you only have one app, like the address book. But since that's effectively just a special case of everything in here, I thought I'd do that as a separate PR once we agreed that we are happy with the basic version

This leaves me inclined to do conflict detection at the route level, not the module level, and make it possible for the application to override the conflict behavior. Sometimes, "halt and catch fire" is the right approach. Other times, pave over this buggy route defined in someone else's library is just the desperate measure you need.

How would that work? Parse the full path? Have the Api register all routes to the App, but bundle each route on the App with the dispatch code for the Api that registered it, essentially including the unpacking and routing as a closure?

I could see that creating ambiguity, if you had /my/module + /route from a normal namespaced module, and /my + /module with a parameter /route from a "global namespace" module, would you find out at runtime?

It should happen in serve, when it registers all the routes with tide. The App would have to check that no global route has the same name as any module. If it failed you'd know because you wouldn't be able to start the server

pictographer · 2022-06-16T19:02:48Z

src/request.rs

+pub struct RequestParams<State> {
+    headers: Headers,
+    state: Arc<State>,
+    params: HashMap<String, RequestParamValue>,


Is there anything tide-disco can do to help with the problem of application route handler code that contains typos in strings used to lookup bindings, etc? Elsewhere in the code, we've used enums with Strum to enable easy conversion between strings and enums so the compiler can help. We can't just use an enum as the HashMap key here because the values aren't known in advance. Maybe we document a recommendation. Not sure whether a wrapper around the HashMap could do anything useful...

We could have a function to look up a parameter like fn param(&self, name: impl Display) -> Option<&RequestParamValue>, and then if you implement Display for your app-specific enum (using Strum, e.g.) you could use it as a key to look up a param.

Also, I think attribute macros can help a lot here. For example you could write a function like

#[tide_disco::route] async fn my_handler(a_required_parameter: TaggedBase64, an_optional_parameter: u128) { ... }

and the route macro could turn this into a function that looks up params named "a_required_parameter" and "an_optional_parameter" and does all the type/error checking for you.

Both solutions are probably good. The macro can just use the param function internally, and I'm a firm believer in macros not doing anything that you couldn't do yourself with the public API, if a bit more verbosely. The param function is easy to do in the short term so I'll make an issue for that.

pictographer · 2022-06-16T20:41:32Z

src/route.rs

+impl<State, Error> Route<State, Error> {
+    /// Parse a [Route] from a TOML specification.
+    pub fn new(name: String, spec: &toml::Value) -> Result<Self, RouteParseError> {
+        unimplemented!("route parsing")


This would be something like,

TryInto::<Route>::try_into(spec)

(?) But then would we get into trouble because the handler hasn't been supplied, or does the fact that it's optional save us?

Exactly, the handler for a new route will always default to None (which causes the default, documentation handler to be invoked if it is still None at runtime) and can later be overridden using at

nyospe

LGTM

nyospe · 2022-06-16T20:49:25Z

src/api.rs

+/// An [Api] is a structured representation of an `api.toml` specification. It contains API-level
+/// metadata and descriptions of all of the routes in the specification. It can be parsed from a
+/// TOML file and registered as a module of an [App](crate::App).
+pub struct Api<State, Error> {


It seems I was mistaken; we will end up with a single State type shared across each Api....

Yep, as discussed we can do something like tide::nest later on but for starters the single global state seems like the better fit for the Espresso APIs

nyospe · 2022-06-16T20:53:10Z

src/api.rs

+    /// handler registered, an error is returned. Note that all routes are initialized with a
+    /// default handler that echoes parameters and shows documentation, but this default handler can
+    /// replaced by this function without raising [ApiError::HandlerAlreadyRegistered].
+    pub fn at<F, Fut, T>(&mut self, name: &str, handler: F) -> Result<&mut Self, ApiError>


Issue for adding a streaming version?

No issue, just wanted to get the basic thing first. I'll create an issue for it

nyospe · 2022-06-16T21:01:03Z

src/route.rs

+    }
+}
+
+fn respond_with<T: Serialize, E>(


I like this approach. 👍

Define the basic server types and interfaces and add a minimal example.

27a8ce4

Closes #13 Closes #14

jbearer requested review from pictographer and nyospe June 15, 2022 23:16

jbearer self-assigned this Jun 15, 2022

pictographer approved these changes Jun 16, 2022

View reviewed changes

nyospe approved these changes Jun 16, 2022

View reviewed changes

This was referenced Jun 16, 2022

Add a function to look up a parameter based on a Display type #25

Closed

Add support for registering a top-level module with no base_url #26

Open

jbearer merged commit 1ba02e7 into main Jun 16, 2022

Ancient123 deleted the feat/interfaces branch June 17, 2022 22:31

This was referenced Jul 15, 2022

Associate a handler from the application with each route #6

Closed

Make it possible to compose application state from multiple indepdenent modules #12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define the basic server types and interfaces and add a minimal example. #21

Define the basic server types and interfaces and add a minimal example. #21

jbearer commented Jun 15, 2022

pictographer left a comment

pictographer Jun 16, 2022

jbearer Jun 16, 2022

pictographer Jun 16, 2022

jbearer Jun 16, 2022

pictographer Jun 16, 2022

jbearer Jun 16, 2022

pictographer Jun 16, 2022

jbearer Jun 16, 2022

jbearer Jun 16, 2022

pictographer Jun 16, 2022

jbearer Jun 16, 2022

pictographer Jun 17, 2022

nyospe Jun 17, 2022

nyospe Jun 17, 2022

jbearer Jun 17, 2022

pictographer Jun 16, 2022

jbearer Jun 16, 2022

pictographer Jun 16, 2022

jbearer Jun 16, 2022

nyospe left a comment

nyospe Jun 16, 2022

jbearer Jun 16, 2022

nyospe Jun 16, 2022

jbearer Jun 16, 2022

nyospe Jun 16, 2022

Define the basic server types and interfaces and add a minimal example. #21

Define the basic server types and interfaces and add a minimal example. #21

Conversation

jbearer commented Jun 15, 2022

pictographer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyospe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment