search

trilemma-dev / SecureXPC

A simple and secure XPC framework for Swift
MIT License
77 stars 15 forks source link

Type errors in routes are conflated with missing routes #20

Closed amomchilov closed 2 years ago

amomchilov commented 2 years ago

With the way the server's routes are looked up today, a type error (e.g. a message is sent expecting a reply, but the server doesn't expect it to need a reply) ends up being diagnosed as routeNotRegistered.

What do you think about centralizing all route storage into a single table? If we find a route with a different type than expected, then we can emit a more specific "type error".

This also has the (beneficial, IMO) side-effect of making routes unique by solely their names, and not their "kinds" (with/without message, with/without reply)

I ran into this when just trying stuff out and copy-pasting a route on the client and server end, at a time when my client and server code didn't have a "shared" package where I could centralize the route definition. With all duplication like that comes the risk of de-synchronization, and indeed, that's what happened.

jakaplan commented 2 years ago

Under the current design this isn't actually a conflation, this is wholly intentional. However, I'm open to changing how this works such that routes are only defined by their paths.

Right now routes are fully typed. A route is both the path and the message and reply types (as applicable). So when the client sent a message expecting a reply, the type of the reply is part of the route. As confusing as it may be, the server was correct in saying such a route was not registered - no conflation actually occurred.

I hope this doesn't come off as me being pedantic or being fixated on semantics, that's not my intention. What I'm trying to convey is the actual behavior you experienced isn't due to imprecise error generation - it's currently exactly the intended behavior.

If we were to have routes be unique solely based on paths, certainly we need one data structure (like a set) to keep track of that. However, due to type constraints we may still need the four dictionaries that exist to wrap the registered functions/closures. I haven't thought this through though, so perhaps that's not necessary.

amomchilov commented 2 years ago

I hope this doesn't come off as me being pedantic or being fixated on semantics

Nope, it makes total sense.

I would compare this with functions in Swift. They're not just identified by their name, but also their parameter and return values. However, notice that the diagnostic you get when you get the params/return type wrong isn't "this function doesn't exist", it will point out specifically that the function exists but the types are wrong.

If we were to have routes be unique solely based on paths, certainly we need one data structure (like a set) to keep track of that. However, due to type constraints we may still need the four dictionaries that exist to wrap the registered functions/closures. I haven't thought this through though, so perhaps that's not necessary.

It's actually doable with a single dictionary (using either dynamic casts or a few conditionals). I'm still working through it, but I'll open a proof-of-concept PR when I get a chance (I'm juggling a lot of things right now heh)