Skip to content

zhr/http request consolidation #18

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 44 commits into
base: develop
Choose a base branch
from

Conversation

Gohlub
Copy link

@Gohlub Gohlub commented Jun 25, 2025

HTTP request routing:

Problem: All HTTP request were routed by JSON body de-serialization, which prevented simple GET requests from going through (and other methods which might not involve parameters).

Solution: a two-phase http routing system for HTTP requests.

In phase one, we first parse parameter-less HTTP requests (those without an argument in the signature) and order the handlers according to how specific they are (specific here meaning whether they have a defined method, path, both, either, or none). This part was a bit tricky since I kept getting errors where I would hit the wrong handler, but after exhaustive testing it seems to work. One important note here is that parameter-less HTTP handlers don't get added to the HPMRequest enum anymore.

Phase 2 is pretty much how it was done before:
Deserializes request body into HPMRequest enum and matches according to the function arguments.

I also made sure to include some more expressive error handling that touches other parts of the code (not strictly related to HTTP).

This PR is related to another PR on the kit WIT parser that I will make shortly.

@Gohlub Gohlub requested a review from nick1udwig June 25, 2025 20:26
Copy link
Member

@nick1udwig nick1udwig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few substantive possible changes, and a few smaller ones

current_path != "/" &&
!current_path.starts_with("/assets") &&
!current_path.starts_with("/static")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we remove this special-casing of ui-related paths? E.g. see #19 as of 62e318a which seems to work fine without the special case

let mut ctx_mut = ctx.borrow_mut();
ctx_mut.current_path = current_path;
ctx_mut.current_http_method = current_method;
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is capturing and restoring context like this required? See 3568d35 which seems to work fine without. If we need it, please add a test that shows how removing this breaks

similar for below

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is capturing and restoring context like this required? See 3568d35 which seems to work fine without. If we need it, please add a test that shows how removing this breaks

similar for below

in my testing, I noticed that without context preservation, even if the handler was routed properly, get_path() would return None, which is not ideal

// Call the websocket handler if it exists
#ws_method_call

// Save state if needed
unsafe {
hyperware_app_common::maybe_save_state(&mut *state);
}
hyperware_process_lib::logging::debug!("WebSocket message processed successfully");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend removing this log

@@ -1006,28 +1545,50 @@ fn generate_message_handlers(
// Save state if needed
hyperware_app_common::maybe_save_state(&mut *state);
}
hyperware_process_lib::logging::debug!("Local message processed successfully");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recommend removing

// Process the local request based on our handlers (now including both local and remote handlers)
match serde_json::from_slice::<HPMRequest>(message.body()) {
Ok(request) => {
hyperware_process_lib::logging::debug!("Successfully deserialized local request");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recommend removing

let mut ctx_mut = ctx.borrow_mut();
ctx_mut.current_path = Some(current_path.clone());
ctx_mut.current_http_method = Some(http_method.clone());
hyperware_process_lib::logging::debug!("Set current_path to: {:?}", ctx_mut.current_path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need both this and L1392: remove one

README.md Outdated
}
```

#### **UI Path Conflicts (Critical!):**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussion in this section seems wrong even if we don't remove the ui path hardcoding

README.md Outdated

### Smart Routing System

The framework now uses intelligent priority-based routing that automatically chooses the best handler based on the request:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

framework now uses

->

framework uses

README.md Outdated
// Set current_http_method to: Some("GET")
```

### Enhanced Error Handling (New!)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove (New!)

README.md Outdated

### Enhanced Error Handling (New!)

The framework now provides much more detailed error messages for debugging:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove now

Copy link
Member

@nick1udwig nick1udwig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More questions: why are we restricting parameter-less #[http]? This PR doesn't have any problems with your test cases (and some code I generated wants to use parameter-less HTTP posts): #20

@nick1udwig
Copy link
Member

If we don't need UI special-casing, PR #19 could be merged as-is. If we want parameter-less #[http], PR #20 needs a little work (i.e. remove commented-out code; update some comments in the code)

@Gohlub
Copy link
Author

Gohlub commented Jun 27, 2025

More questions: why are we restricting parameter-less #[http]? This PR doesn't have any problems with your test cases (and some code I generated wants to use parameter-less HTTP posts): #20

I am not sure what you mean? The macro supports them well (as per the testing app I showed you) and I updated the WIT parser to handle them as well.

@nick1udwig
Copy link
Member

TODOs:

  • a RESPONSE_REGISTRY-like method for saving state that must survive an await
  • passing "long-lived HTTP request" test add long-lived test Gohlub/http-testing-ground#1
  • have thought about other long-lived request cases carefully and put state as appropriate

@nick1udwig nick1udwig mentioned this pull request Jul 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants