clj-commons / aleph

Asynchronous streaming communication for Clojure - web server, web client, and raw TCP/UDP
http://aleph.io
MIT License
2.54k stars 241 forks source link

Update schema to accept string HTTP methods like "GET", as well as keywords. #685

Open danielcompton opened 1 year ago

danielcompton commented 1 year ago

The changes in release 0.6.2 broke some production code which was using :request-method "GET" instead of :request-method :get. I thought it might be good to warn others about this.

KingMob commented 1 year ago

@danielcompton Thanks for pointing this out. At first, I was surprised that string methods even worked, but I took a look at the code, and the transformation from keyword to Netty input is such that strings will usually work, too. (Though string TRACE methods will break some code.) FWIW, the Ring SPEC is clear, and it's been keywords-only for its entire life.

Regardless, I guess the question is, should we:

  1. Add a breaking change note, like this PR?
  2. Remove wrap-validation from the default middleware, and silently continue to accept strings?
  3. Loosen the schema to accept strings, too?

I'd rather not break any other wild code, so I prefer 2 or 3. I think 2 will leave the validation effectively unused by 99.99% of users, which defeats its purpose. I still hope to tighten the schema for areas like the query-string and the uri, so I vote for 3. This is not a strongly-held opinion, though. Thoughts?


From #679:

We should probably tighten the schemas soon. I've had bad experiences in the past using spec, when people let specs be loose for too long, they became concerned that locking them down would break something (E.g., one team only accepts ports as numbers, but another team also accepts them as strings which it parses...)

Isn't it too late already?

Well, that comment exchange proved prophetic 😂

danielcompton commented 1 year ago

I'd rather not break any other wild code, so I prefer 2 or 3. I think 2 will leave the validation effectively unused by 99.99% of users, which defeats its purpose. I still hope to tighten the schema for areas like the query-string and the uri, so I vote for 3. This is not a strongly-held opinion, though. Thoughts?

Yeah, 3 seems like a safe middle ground without breaking existing code when people upgrade Aleph versions.

A possible extension to 3 could be to log a warning when callers pass a string instead of a keyword. That way, you could still tighten the spec one day if you ever decided to. Although I don't really know what you'd be gaining at that point, so maybe allowing string + keyword is ok.

arnaudgeiser commented 1 year ago

Loosen the schema to accept strings, too?

I would definitely loosen the schema. And as a follow-up, confirm we didn't break any other parameters along the way.

KingMob commented 1 year ago

Option 3 it is.

Daniel, do you want to tackle it, since you have the PR open? It's fine if not, I'm just saying you're welcome to if you like.

danielcompton commented 1 year ago

Sorry, I dropped this. Happy for you to take it if you'd like.

KingMob commented 1 year ago

@danielcompton We can do it, but I have plenty on my plate at the moment, and you're welcome to if you like. Just let us know.

danielcompton commented 6 months ago

@KingMob I've updated the PR to expand the validation. LMK what you think.