-
Notifications
You must be signed in to change notification settings - Fork 5
docs: Design typed pipeline #74
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
base: main
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@coderabbitai could you review the design document in DESIGN_typed_context_pipeline.md |
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed:
Length of output: 18902 @nielsenko I've reviewed the DESIGN_typed_context_pipeline.md document and I'm impressed with the thoughtful approach to implementing a typed context pipeline in Relic. Overall AssessmentThis is an excellent design document that presents a clever solution to the common problem of ensuring middleware is correctly composed. The use of Dart's type system, particularly extension types, to catch errors at compile-time rather than runtime represents a significant improvement over traditional approaches. Strengths
Suggestions for Enhancement
Technical ImplementationThe core approach using extension types, Expando, and generic composition is solid. The This design represents a sophisticated use of Dart's type system that should significantly improve developer experience by catching errors early and providing clear, type-safe access to context data. |
Co-authored-by: Timm Preetz <[email protected]>
da468b6
to
e253ee5
Compare
- Move DESIGN_typed_context_pipeline into design folder - Add runnable simplified example code in design/appendix_a.dart
771b3a8
to
935e0dd
Compare
@coderabbitai Added extra section on routing as well as a larger runnable sample in appendix_a.dart. Could update your review? |
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@nielsenko Thank you for the updates! I've reviewed the new section on routing and the appendix_a.dart sample. Updated AssessmentThe additions significantly strengthen the design document and address several of my previous suggestions: New Routing SectionThe routing section elegantly demonstrates how the typed context pipeline integrates with HTTP routing mechanisms. I appreciate how it shows:
This addresses my previous suggestion about complex scenarios by showing how the pipeline works in real routing situations. Appendix A Sample CodeThe appendix_a.dart sample is an excellent addition that:
This sample effectively serves as both documentation and proof-of-concept, addressing my suggestion about complex scenarios and implementation details. Remaining Enhancement OpportunitiesWhile the new additions address several of my previous suggestions, a few areas could still be enhanced:
Overall, these additions make the design document substantially more complete and practical. The combination of theoretical explanation, practical examples, and runnable code creates a comprehensive guide that should be very valuable to developers implementing this pattern. 🏁 Script executed:
Length of output: 483 🏁 Script executed:
Length of output: 35513 🏁 Script executed:
Length of output: 9679 @nielsenko Thank you for the updates! I've reviewed the new section on routing and the appendix_a.dart example. Updated AssessmentThe additions substantially enhance the design document and address several of my previous suggestions. New Routing SectionThe routing integration section is a valuable addition that demonstrates:
This effectively addresses my previous suggestion about handling complex scenarios by showing how routing fits into the overall design. Appendix A ExampleThe runnable example in appendix_a.dart is particularly impressive and brings the design to life:
This implementation example makes the abstract concepts concrete and provides a complete reference for developers implementing the pattern. Remaining Enhancement OpportunitiesWhile these additions significantly strengthen the design, a few minor areas could still be enhanced:
Overall, these additions have elevated an already strong design document into a comprehensive guide. The combination of theoretical explanation, practical examples, and runnable code creates an excellent reference for developers implementing this pattern. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I applaud what you're trying to achieve here 😀
It's really unfortunate that many of the "obvious" things we would want to do with generics here (like having an add that is ExistingMiddleware & T addMiddleware(Middleware<T> middleware)
) is currently not possible.
I was not a big fan of the mixin
s in each route, but I wonder if the higher-level like a Router
/ Pipeline
is exactly where that would make sense and solve a ton of type-issues and where stuff would only have to be added/changed in a single place.
(Having applications subclass Pipline with A, B, C
is probably the easiest one, another might be to have an explicit middleware step (method) that outputs a new context (by calling into the middleware). Offers more flexibility, but less out of the box "speed".)
Just my 2 cents, will have to ponder this for a bit.
Also many of the ideas that came to my mind for how to handle this, just don't work out nicely in practice…
Looking forward to having a chat about this :)
|
||
final _adminRoleProperty = ContextProperty<AdminRole>('admin_role'); | ||
extension type AdminContextView(RequestContext _relicContext) | ||
implements UserContextView { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was surprised this would even compile, because the type does not conform to UserContextView
.
And indeed when I remove the userView
from adminAuthMiddleware
, it still compiles, but then fails at runtime:
Unhandled error: Bad state: Property user not found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow.
xtension type AdminContextView(RequestContext _relicContext) { // missing implements
will introduce a compile error in:
FutureOr<Response> handleAdminDashboard(final AdminContextView context) {
print(
'Handling Admin Dashboard for ${context.user.name} (${context.adminRole.roleName})');
return Response.ok(
'Admin: ${context.user.name}, Role: ${context.adminRole.roleName}');
}
The meaning of implements for extension types are defined here.
In particular:
An extension type can only implement:
...
- Another extension type that is valid on the same representation type. This allows you to reuse operations across multiple extension types (similar to multiple inheritance).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I defined:
extension type AdminContextView._(RequestContext _relicContext)
implements UserContextView {
AdminContextView(UserContextView uv) : _relicContext = uv._relicContext;
// Admin also has User
AdminRole get adminRole => _adminRoleProperty.get(_relicContext);
void attachAdminRole(final AdminRole role) =>
_adminRoleProperty.set(_relicContext, role);
}
would that be better?
In general the example don't do full encapsulation. You could argue the ctor should take AdminRole
as well and do the attach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow.
I did not mean removing the implements
keyword, but rather the implementation (of assigning the user ID inside the admin context view).
I can remove the implementation, get it to compile and then error at runtime.
(We discussed this already, just for completeness sake here.)
AdminContextView(UserContextView uv)
in the constructor seems like a better pattern to me, pushing the implementation to be more correct. Sadly that only solves 1 layer of inheritance. If you wanted to build a combined view that had implements A, B
that would not work as well (unless you take 2 constructor params and hope that they point fo the same context…).
} | ||
|
||
// Admin Auth: Adds User and AdminRole, returns AdminContextView | ||
AdminContextView adminAuthMiddleware(final BaseContextView inputView) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think with the AdminContext
implementing the UserContext
, this example is a bit too simple.
Because there it "makes sense" that one could implement the other.
But assume we had a "client language" context (which reads the HTTP header) instead. Then it would feel weird to that in the pipeline of
.add(logger)
.add(clientLanguage)
.add(user)
We would have to wrap/modify the user
middleware to also include the language, right?
Unless I am mistaken on this point, I think it's a bit of a code-smell if in order to add a new middleware I would have to change 2 places in tandem.
Maybe then it would be better if (for example, just a spitball) a router/server was an abstract class to instantiate for ones specific project, which would use mixins
with all the middleware, and then the route could just depend on it (and routes provided from third party packages would require you to add that mixin to provide the right context before being able to "mount" them – or they use a subrouter which has those only for the affected routes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 let's discuss
} | ||
|
||
// === ContextProperty and Views Stubs === | ||
class ContextProperty<T extends Object> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can get behind this pattern.
I think such a declaration is very smart, because it gives you a unique identifier and here (unlike in riverpod
etc.) it's only the helper / identifier instance and does not practically have any global state (as for the you need the specific request context / token).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this pattern makes sense with the current paradigm as well.
* It provides access to the current `Request` object (e.g., via a `request` getter). | ||
* It contains a stable `token` (e.g., via a `token` getter). This `token` is a unique identifier for the entire lifecycle of a single request and remains constant even if the `RequestContext` instance itself undergoes state transitions (e.g., from `NewContext` to `ResponseContext`). | ||
|
||
All request-scoped data attached via `Expando` (managed by `ContextProperty`) will be keyed off this stable `requestToken`. The extension type views defined in this design will wrap an instance of Relic's `RequestContext`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the performance characteristics with using the various expandos. I think we could similarly have just the state on the context / token itself, and then provide getter / setter like V get(ContextProperty<V> prop)
on it.
I like how easy it is in the current approach to hide the setter though (just making that part file-local for example), and wonder if replicating this would be much harder in my above suggestion (e.g. you could have something like Completer
, with an "value reader" and "value setter" being distinct, and only 1 exported as a direct helper).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Todays Expando
s are as about as fast as HashMap
s. As a context bag would be implemented with a map as well, it is not clear to me what would be faster.
BTW, this is abstracted by the ContextProperty<T>
so we can "easily" change it, if any speed gain mandate.
final _sessionProperty = ContextProperty<Session>('relic.session'); | ||
|
||
// A composite view indicating both User and Session information are available. | ||
extension type UserSessionContextView(RequestContext _relicContext) implements UserContextView, SessionContextView { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the Achilles' heel of the proposal.
While the application could easily build "the one full context" (per router), how would this combine with externally provided middle?
Wouldn't I have to wrap each step in a *ContextView
that threads through all the previous one (specific to my current app configuration)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you have to define these combinators, even if they are trivial.
if only we could do:
(T with FancyContext) middleware<T extends BaseContextView>(T context) => ...; //
ie. return a unnamed type that is the composition of another type with a mixin.
* Handlers and middleware can declare precisely the context view (and thus data) they expect. | ||
* Access to context data via extension type getters is type-safe and clear (e.g., `context.user`). | ||
* **Minimal Runtime Overhead for Views**: Extension types are intended to be zero-cost compile-time wrappers. The `ContextProperty` helper encapsulates `Expando` lookups/attachments, which are generally efficient. | ||
* **No Class Explosion**: Avoids needing a distinct context `class` for every possible combination of middleware. Extension types provide views, and `ContextProperty` manages data association with the stable `requestToken`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoids needing a distinct context
class
for every possible combination of middleware.
Is this true though? Maybe I misunderstand some aspect of the implementation or how I could avoid that, but I feel like I would always need to make the "next" middleware aware of the previous (and thus alter it's type when pre-pending a new one). (Assuming all middleware enhances the context with local data and is not just a passthrough like the logger in this example.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no class explosion in the sense that context views are zero cost extension types. They have no runtime representation.
|
||
This typed pipeline introduces a shift from the traditional `Middleware = Handler Function(Handler innerHandler)` pattern previously used. Understanding these changes is crucial: | ||
|
||
* **New Middleware Signature**: Middleware in this design are functions with a signature like `OutputView Function(InputView)`. They transform context views rather than wrapping an inner handler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting here would be awesome 🤩
* **Linear Chain**: The `PipelineBuilder` composes middleware into a linear chain of context transformations. Each middleware is expected to process the context and pass control (via its return value) to the next stage defined by the builder. | ||
* **Short-Circuiting (e.g., Denying Access)**: | ||
* Middleware should not directly return a `Response` to short-circuit the pipeline. | ||
* Instead, if a middleware needs to stop processing and return an error (e.g., an authentication middleware denying access due to an invalid token), it should **throw a specific exception** (e.g., `AuthorizationRequiredError("Invalid token")`, `PaymentRequiredError()`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tricky, personally I was not a fan of the throw
.
But maybe this could be handle with some Result
-ish type which distinct between "early return" and "next".
But either way, I think that is a secondary consideration and would not need to happen in combination with the major change of this proposal, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, having just fought to get rid of HijackException
based control flow I know where you are coming from.
However, you won't inadvertently catch these exceptions in the pipeline as the middleware don't wrap each other as today.
I kind of made peace with it. Passing a context to a handler that it shouldn't react to is also a bit weird, and then there is the question of typing.
* **Boilerplate for `ContextProperty` and Views**: Each new piece of context data requires defining a `ContextProperty` instance and corresponding view methods. However, this is more structured and less error-prone than raw `Expando` usage. | ||
* **Learning Curve**: Developers using the framework will need to understand context views, `ContextProperty`, the role of `requestToken`, the pipeline builder, and the implications of the new middleware paradigm. | ||
* **Discipline with `requestToken`**: The `ContextProperty` helper ensures that data is keyed off the stable `token` within the `RequestContext`, mitigating direct misuse of `Expando`s with transient `RequestContext` instances themselves as keys. | ||
* **Middleware Return Types**: Middleware authors must be careful to return the correct context view type that accurately reflects the data they've attached via `ContextProperty` and the `requestToken`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this already refers to the issue I have encountered below, where I could get the code to compile but the middleware implementation to be wrong and err at runtime.
I suppose "just" needing to take care in the middleware implementation (which should mostly be small) is still a win over having to manually match types across the entire router setup though.
So maybe this is the right trade-off. For comparison though, maybe we should take a look at what costs a fully type-safe middleware implementation would really entail. E.g. I imagine that even a simple middleware like an auth check or custom logger would access a network store (to validate the session), or do a big computation (to verify a token), such that creating a single class instance to store/wrap the result would not be a practical slow-down at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking forward to discuss your thoughts on this.
Description
This PR introduces a design for a new typed pipeline system in Relic. The goal is to improve both developer ergonomics (clear, type-safe access to context data like
context.user
) and compile-time safety, preventing errors from misconfigured middleware. The core idea involvesContextProperty
, extension type views, and a generics basedPipelineBuilder
. See DESIGN_typed_context_pipeline.md.