Skip to content
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

Coercion around a route causes custom match extension records to be lost (affects vhosts) #187

Open
SevereOverfl0w opened this issue Sep 5, 2018 · 1 comment

Comments

@SevereOverfl0w
Copy link
Contributor

SevereOverfl0w commented Sep 5, 2018

(defrecord A [])
((schema.coerce/coercer bidi.schema/RoutePair {})
 [(->A) :match])

Should result in an unchanged result, but actually results in [{} :match], which silently breaks the route structure. This seems to be schema behavior, see:

((schema.coerce/coercer {schema.core/Any schema.core/Any} {})
 (->A))
;; => {}

I think the correct change here is to undo part of ca14e6e and re-add (s/protocol bidi/Pattern) to the schema. However I don't know that I understand why it was commented out. Maybe @griff can weigh in on that?

This bug prevents vhosts from working with custom pattern matchers, as it relies on coercion in order to function.

@griff
Copy link
Contributor

griff commented Sep 6, 2018

Most of my reasoning for making the Schema more strict is explained here #130 but in essence bidi/Pattern is implemented for all maps, sets and vectors and therefore when the schema contains (s/protocol bidi/Pattern) it will accept ANY map, set or vector regardless of whether they are valid bidi patterns or not making the schema pretty useless as a validator.

My original PR was meant to start a discussion about what a valid bidi pattern is and how to validate that. For instance custom pattern matchers are not part of the BNF for bidi patterns as they are described in the README but they clearly should be. One way to support custom pattern matchers while still leaving the schema strict is to have separate protocols for internal and custom pattern matchers so either saying bidi/Pattern is an internal protocol and custom pattern matchers should instead implement a new bidi/PatternExtension protocol or renaming the current bidi/Pattern to bidi/InternalPattern everywhere and having a bidi/Pattern protocol for custom pattern matchers.

SevereOverfl0w added a commit to SevereOverfl0w/bidi that referenced this issue Apr 24, 2019
This breaks custom matchers, as mentioned in juxt#187.  This does not fix
 juxt#187 because RoutePair generally needs fixing, but this makes it
sufficiently optional for users.
SevereOverfl0w added a commit to SevereOverfl0w/bidi that referenced this issue Apr 24, 2019
This breaks custom matchers, as mentioned in juxt#187.  juxt#187 is not fixed
because RoutePair generally needs fixing, but this makes it sufficiently
optional for users.
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

No branches or pull requests

2 participants