Add URL wildcard feature (fixes #47) #53

jmileham · 2023-07-13T13:49:17Z

/domain @samandmoore @Betterment/webvalve-core
/platform @samandmoore @Betterment/webvalve-core

Regexp felt too powerful and would've been a major breaking change, so here's an attempt to sneak in wildcard * support without changing the syntax too much. I'll call out some interesting bits in comments.

spec/webvalve/manager_spec.rb

jmileham

Here are my notes!

spec/webvalve/manager_spec.rb

spec/webvalve/service_url_converter_spec.rb

spec/webvalve/manager_spec.rb

spec/webvalve/service_url_converter_spec.rb

README.md

lib/webvalve/service_url_converter.rb

jmileham · 2023-07-13T21:45:09Z

README.md

@@ -218,6 +218,28 @@ WebValve.register FakeBank, url: ENV.fetch("SOME_CUSTOM_API_URL")
 WebValve.register FakeBank, url: "https://some-service.com"
 ```

+## Mocking dynamic URLs with wildcards


Mocking is probably not the right word - what is?

I might just say "Using wildcards for dynamic URLs"

jmileham · 2023-07-13T22:41:53Z

Barring additional feedback on my newer changes, I think the major outstanding question is whether we're comfy with the level of grossness that the suffix pattern adds to every single URL pattern in webmock. How bad is it that http://foo.co will also match requests to http://foo.com if we don't add it? I can imagine it being very annoying for some kind of app integrating hundreds of services, like an API aggregator of some kind, but it's probably mostly not going to have security repercussions?

jmileham · 2023-07-13T22:43:37Z

mostly correctness repercussions, though, and if you have a couple similar domains/apps it could really mess you up until you figure out what's going on.

jmileham · 2023-07-13T22:45:38Z

Maybe it happens more around path suffixes honestly if you're treating a bunch of subtrees of an app as different webmock domains.

jmileham · 2023-07-14T13:03:55Z

Another sub-consideration: if we decide to keep the suffix pattern, since it's a breaking change that somebody might have been taking advantage of (e.g. including the static part of a dynamic token at the end of the URL) is it worth putting a fail-safe change management feature in there, e.g. encoding a WebValve.strict_suffix_matching = true into the generated config and falling back to the non-strict suffix matching? Or is the current behavior better understood as a bug?

Relatedly, you could make the case that the wildcard pattern is a breaking change worthy of a similar fail-safe upgrade pattern because you used to be able to use the * literal in your URLs and now it's a metacharacter unless you escape it both in the webvalve config, and probably in app use. I think that's vanishingly unlikely and not worth doing, but just calling it out.

jmileham · 2023-07-14T13:26:27Z

I took a stab at a post_install_message that discloses both behavior changes and bumped version to 2.0 because of the breaking changes.

jmileham · 2023-07-14T13:51:59Z

If we wanted to go in an entirely different direction we could pursue Addressable templates instead, which would probably look cleaner and have some of the safety I had to engineer into this by default. Addressable templates (mostly? completely?) wrap their dynamic sections in curlies which are relatively unlikely to be present unencoded in URLs.

jmileham · 2023-07-14T13:54:21Z

Notably with URL templates we'd either have to suffix user-provided URL templates dynamically to enable them to support arbitrary suffixes, or people would have to provide their own suffixes. Could get finicky. The UX of wildcarding is probably easier to understand for somebody who doesn't slice and dice URLs at a framework level for a living.

jmileham · 2023-07-14T14:23:40Z

Oh maybe middle ground: use wildcard syntax but render as addressable templates? Might be quite a bit fancier on the inside because the template representation of * would likely change by context, but that might be doable, and the resulting template is probably more legible than a hairy regexp.

lib/webvalve/version.rb

samandmoore · 2023-07-17T14:07:48Z

Barring additional feedback on my newer changes, I think the major outstanding question is whether we're comfy with the level of grossness that the suffix pattern adds to every single URL pattern in webmock. How bad is it that http://foo.co will also match requests to http://foo.com if we don't add it? I can imagine it being very annoying for some kind of app integrating hundreds of services, like an API aggregator of some kind, but it's probably mostly not going to have security repercussions?

I'm fine with being more restrictive. I think that adding the trailing * is a relatively easy fix for anyone in that situation.

samandmoore · 2023-07-17T14:09:41Z

Another sub-consideration: if we decide to keep the suffix pattern, since it's a breaking change that somebody might have been taking advantage of (e.g. including the static part of a dynamic token at the end of the URL) is it worth putting a fail-safe change management feature in there, e.g. encoding a WebValve.strict_suffix_matching = true into the generated config and falling back to the non-strict suffix matching? Or is the current behavior better understood as a bug?

I consider the behavior a bug. Given that there will be a way to restore the behavior, I'm fine with making the breaking changes and doing a major version bump.

Relatedly, you could make the case that the wildcard pattern is a breaking change worthy of a similar fail-safe upgrade pattern because you used to be able to use the * literal in your URLs and now it's a metacharacter unless you escape it both in the webvalve config, and probably in app use. I think that's vanishingly unlikely and not worth doing, but just calling it out.

Agreed

lib/webvalve/service_url_converter.rb

samandmoore · 2023-07-18T14:38:38Z

lib/webvalve/service_url_converter.rb

@@ -2,48 +2,76 @@
 require 'addressable/template'

 module WebValve
+  # @api private


this reminds me, i gotta do a pass over this library to call private_constants. will make a note for myself for that too

spec/webvalve/service_url_converter_spec.rb

samandmoore

domainlgtm platformlgtm

jmileham · 2023-07-18T15:02:00Z

@ABaldwinHunter since you proposed issue #47 and @slondr since you upvoted, does this feature scratch the itch you're looking to scratch? We're ready to pull the trigger on it but want to do a final check that we didn't build the wrong thing. :) See the README diff for the TL;DR.

* origin/main: Bump webvalve to 1.3.0 Drop support for older rails and rubies (#54)

samandmoore

domainlgtm

jmileham · 2023-07-19T18:30:34Z

Addressable::Templates turn out to be too strict in practice to be a general purpose URL matcher for Webvalve. URLs like http://foo.com/?bar don't match http://foo.com/{?query*} because there's no = in the query param. URLs like http://foo.com/?bar=baz,bump don't match because , is a reserved character in the RFCs despite it being commonly used on the wire. So I'm going to revert some commits here to get back to the Regexp strategy, which will result in less legible URL patterns in webmock, but we'll live.

This reverts commit 878fea0.

This reverts commit d1f8cb5.

This reverts commit 6ed0170.

This reverts commit e20c482.

This reverts commit 96c0aed.

This reverts commit cb35fe3.

This reverts commit ad2775c.

This reverts commit 2deb420.

jmileham · 2023-07-19T20:07:48Z

Ok, this PR, back in Regexp-backed form works with one of our largest codebases. @ABaldwinHunter @slondr please hit us with feedback but the fact that in spite of the slight behavior changes it didn't break a big app that heavily uses webvalve also gives me confidence that this is not a one way door.

samandmoore

domainlgtm platformlgtm

samandmoore

domainlgtm platformlgtm

Add URL wildcard feature (fixes #47)

4e61b33

smudge reviewed Jul 13, 2023

View reviewed changes

spec/webvalve/manager_spec.rb Show resolved Hide resolved

jmileham added 3 commits July 13, 2023 11:53

Replace confusing/inaccurate test

c83dd6b

Add another illustrative test of basic auth behavior

d8c8990

Correctly describe test

0c00267

jmileham commented Jul 13, 2023

View reviewed changes

jmileham added 3 commits July 13, 2023 12:59

collapse unused constant

2f8c767

Rename general purpose boundary chars away from wildcard

4f79759

Simplify boundary chars

29ca620

jmileham commented Jul 13, 2023

View reviewed changes

spec/webvalve/service_url_converter_spec.rb Show resolved Hide resolved

samandmoore reviewed Jul 13, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

samandmoore reviewed Jul 13, 2023

View reviewed changes

lib/webvalve/service_url_converter.rb Outdated Show resolved Hide resolved

jmileham added 4 commits July 13, 2023 15:03

Fix bug with trailing slashes

311ceb3

extra newline

790c1a2

test double-star behavior

f84fa07

moar tests

243bb5c

jmileham commented Jul 13, 2023

View reviewed changes

jmileham force-pushed the jmileham/url_wildcards branch from ebd8901 to c59fe26 Compare July 14, 2023 13:26

jmileham force-pushed the jmileham/url_wildcards branch from c59fe26 to 6b9c8a9 Compare July 14, 2023 13:27

smudge reviewed Jul 16, 2023

View reviewed changes

lib/webvalve/version.rb Outdated Show resolved Hide resolved

jmileham added 4 commits July 17, 2023 22:48

more succinct

96c0aed

also more succinct

e20c482

Update README to match Addressable::Template capabilities

9c79ef2

Better error message

6ed0170

samandmoore reviewed Jul 18, 2023

View reviewed changes

Better comment

d1f8cb5

jmileham force-pushed the jmileham/url_wildcards branch from 0d186e9 to d1f8cb5 Compare July 18, 2023 14:44

samandmoore approved these changes Jul 18, 2023

View reviewed changes

Update spec/webvalve/service_url_converter_spec.rb

8cb1bf5

samandmoore and others added 2 commits July 18, 2023 11:04

Merge remote-tracking branch 'origin/main' into jmileham/url_wildcards

0cca545

* origin/main: Bump webvalve to 1.3.0 Drop support for older rails and rubies (#54)

Match multiple query params!

878fea0

samandmoore approved these changes Jul 18, 2023

View reviewed changes

jmileham added 9 commits July 19, 2023 14:31

Revert "Match multiple query params!"

af321c6

This reverts commit 878fea0.

Revert "Better comment"

de5ff78

This reverts commit d1f8cb5.

Revert "Better error message"

3332cf7

This reverts commit 6ed0170.

Revert "also more succinct"

b8cf5c1

This reverts commit e20c482.

Revert "more succinct"

eb135e9

This reverts commit 96c0aed.

Revert "Typo in code comments"

5a9692d

This reverts commit cb35fe3.

Revert "Factor out #template for legibility"

1719ed5

This reverts commit ad2775c.

Revert "Switch to Addressable::Template strategy"

3f2bcb0

This reverts commit 2deb420.

Don't rely on Addressable for tests when it's not a dependency

94da985

samandmoore approved these changes Jul 20, 2023

View reviewed changes

jmileham and others added 2 commits July 20, 2023 11:21

Merge branch 'main' into jmileham/url_wildcards

ba8de5f

2.0.0 changelog entry

b0b8c17

samandmoore approved these changes Jul 20, 2023

View reviewed changes

samandmoore merged commit d8668c5 into main Jul 20, 2023
10 checks passed

samandmoore deleted the jmileham/url_wildcards branch July 20, 2023 15:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add URL wildcard feature (fixes #47) #53

Add URL wildcard feature (fixes #47) #53

jmileham commented Jul 13, 2023

jmileham left a comment

jmileham Jul 13, 2023

samandmoore Jul 17, 2023

jmileham commented Jul 13, 2023 •

edited

Loading

jmileham commented Jul 13, 2023

jmileham commented Jul 13, 2023

jmileham commented Jul 14, 2023

jmileham commented Jul 14, 2023

jmileham commented Jul 14, 2023

jmileham commented Jul 14, 2023

jmileham commented Jul 14, 2023

samandmoore commented Jul 17, 2023

samandmoore commented Jul 17, 2023

samandmoore Jul 18, 2023

samandmoore left a comment

jmileham commented Jul 18, 2023 •

edited

Loading

samandmoore left a comment

jmileham commented Jul 19, 2023

jmileham commented Jul 19, 2023

samandmoore left a comment

samandmoore left a comment

Add URL wildcard feature (fixes #47) #53

Add URL wildcard feature (fixes #47) #53

Conversation

jmileham commented Jul 13, 2023

jmileham left a comment

Choose a reason for hiding this comment

jmileham Jul 13, 2023

Choose a reason for hiding this comment

samandmoore Jul 17, 2023

Choose a reason for hiding this comment

jmileham commented Jul 13, 2023 • edited Loading

jmileham commented Jul 13, 2023

jmileham commented Jul 13, 2023

jmileham commented Jul 14, 2023

jmileham commented Jul 14, 2023

jmileham commented Jul 14, 2023

jmileham commented Jul 14, 2023

jmileham commented Jul 14, 2023

samandmoore commented Jul 17, 2023

samandmoore commented Jul 17, 2023

samandmoore Jul 18, 2023

Choose a reason for hiding this comment

samandmoore left a comment

Choose a reason for hiding this comment

jmileham commented Jul 18, 2023 • edited Loading

samandmoore left a comment

Choose a reason for hiding this comment

jmileham commented Jul 19, 2023

jmileham commented Jul 19, 2023

samandmoore left a comment

Choose a reason for hiding this comment

samandmoore left a comment

Choose a reason for hiding this comment

jmileham commented Jul 13, 2023 •

edited

Loading

jmileham commented Jul 18, 2023 •

edited

Loading