Extend URI Parsing Specs #69

Tensho · 2017-12-11T10:16:43Z

Before we adopt URI query params in bunny and amqp, I'd like to polish specs for URI in amq-protocol. While I was investigating the code, I met some non-intuitive places that would like to discuss. I guess it's better to talk about them here in PR as far as I tackled required places, or at least start here and move to GitHub issues if needed.

Tensho · 2017-12-11T11:00:03Z

lib/amq/settings.rb

-          # server
-          :host  => "127.0.0.1",
-          :port  => AMQ::Protocol::DEFAULT_PORT,
+        # Consider to move this defaults to client libraries (bunny, amqp, auto-generated amq-protocol client)


AMQ::Settings only holds defaults for the assumed clients and switch between Hash and String like options processing. In my opinion, all client specific parameters should be handled (validate, fallback to default, other conditional logic, etc.) exactly inside client code, not protocol library.

I'd like to propose to retire client specific options (frame_max) form AMQ::Settings module and leave only basic ones. "Basic" means all that standard Erlang client supports according to "URI Specification" and "URI query parameters" official RabbitMQ documentation page.

I doubt that localhost host or guest username should be setup at this abstraction layer. Instead they should be specified as the defaults on the client library side for the better convenience to use it.

If all clients use the same defaults, why extract anything?

For the sake of concerns separation. Underlying protocol library is not responsible for client specific features and should adopt them as plugins/extensions. The same is reasonable for the configuration I guess.

If the settings are identical, are they really client-specific? What's the probability of them changing independently (e.g. in only one client or both to different values)? I'd say it's 0. Please stick to the changes we've discussed," concern separation" is a fantastic way of wasting a lot of time on something that you'd never get any real benefit from.

OK :) I don't mind to stick with the current design, just announced my thoughts. I will proceed with bunny and amqp. Could we merge the specs extension in this PR anyway?

Tensho · 2017-12-11T11:15:19Z

lib/amq/uri.rb

@@ -6,7 +6,10 @@
 module AMQ
  class URI


What do you think if we try to conform URI Ruby standard library hierarchy and extract separate AMP::URI::AMQP and AMP::URI::AMQPS classes?

I don't see any real benefit.

It could simplify the code and make it more object-oriented instead of procedural in my opinion. Because AMQP URI is just another kind of generic URI, contributors, that are familiar already with URI (especially as it's a standard library), intuitively could rely on known public API.

I don't see how having 4 new classes would simplify things. We are not preparing this library for inclusion into the standard library.

Moving classes and constants around means Bunny and amqp gem will have to be updated, and their older versions won't be compatible with newer version of amq-protocol, which has been the case for years.

Sorry but "more object-oriented code" for a couple of constants is a solution in search of a problem.

I see, appreciate your open point of view, thank you 🙇

michaelklishin · 2017-12-11T15:41:56Z

lib/amq/settings.rb

-          # server
-          :host  => "127.0.0.1",
-          :port  => AMQ::Protocol::DEFAULT_PORT,
+        # Consider to move this defaults to client libraries (bunny, amqp, auto-generated amq-protocol client)


This comment should be deleted as these defaults won't move to Bunny and amqp gem.

michaelklishin · 2017-12-11T15:44:25Z

spec/amq/uri_parsing_spec.rb

+          let(:uri) { "amqp://" }
+
+          # Note that according to the ABNF, the host component may not be absent, but it may be zero-length.
+          it "fallbacks to default nil host" do


"it falls back"

Got it, will fix

michaelklishin · 2017-12-11T15:44:51Z

spec/amq/uri_parsing_spec.rb

+        context "schema amqp" do
+          let(:uri) { "amqp://rabbitmq" }
+
+          it "fallbacks to 5672 port" do


Same as above.

michaelklishin · 2017-12-11T15:44:56Z

spec/amq/uri_parsing_spec.rb

+        context "schema amqps" do
+          let(:uri) { "amqps://rabbitmq" }
+
+          it "fallbacks to 5671 port" do


michaelklishin · 2017-12-11T15:45:09Z

spec/amq/uri_parsing_spec.rb

+      context "only username present" do
+        let(:uri) { "amqp://alpha@rabbitmq" }
+
+        it "parses user and fallbacks to nil pass" do


michaelklishin · 2017-12-11T15:45:16Z

spec/amq/uri_parsing_spec.rb

+        context "with ':'" do
+          let(:uri) { "amqp://alpha:@rabbitmq" }
+
+          it "parses user and fallbacks to "" (empty) pass" do


michaelklishin · 2017-12-11T15:45:24Z

spec/amq/uri_parsing_spec.rb

+      context "only password present" do
+        let(:uri) { "amqp://:beta@rabbitmq" }
+
+        it "parses pass and fallbacks to "" (empty) user" do


michaelklishin · 2017-12-11T15:45:32Z

spec/amq/uri_parsing_spec.rb

+        context "with ':'" do
+          let(:uri) { "amqp://:@rabbitmq" }
+
+          it "fallbacks to "" (empty) user and "" (empty) pass" do


michaelklishin · 2017-12-11T15:46:01Z

spec/amq/uri_parsing_spec.rb

+        end
+      end
+
+      context "escaped" do


I believe you mean "%-encoded".

According to method names in URI::Escape module of URI library, escaped and encoded mean the same. But your variant looks more descriptive and unambiguous ^_^

new: - heartbeat - connection_timeout - channel_max - auth_mechanism - verify - fail_if_no_peer_cert - cacertfile - certfile - keyfile rename: timeout -> connection_timeout

Tensho

BTW, if amq-protocol is intended to handle defaults for bunny and amqp, why does Bunny::Session has it's own default constants, like DEFAULT_HOST, DEFAULT_PASSWORD, DEFAULT_HEARTBEAT, DEFAULT_CHANNEL_MAX?

…sion

michaelklishin · 2017-12-11T22:30:43Z

Because Bunny is several years older than amqp-protocol.

Percent encoding only applies to virtual hosts (URI paths). CGI.unescape is the only non-deprecated way of %-decoding string values so it can be applied to hostnames but different parts of URI use different encoding schemes. References #69.

## Changes between 2.2.0 and 2.3.0 (Jan 8th, 2018) ### Support for Additional URI Query Parameters GitHub issue: [#67](ruby-amqp/amq-protocol#67), [#68](ruby-amqp/amq-protocol#68), [#69](ruby-amqp/amq-protocol#69). Contributed by Andrew Babichev.

Extend URI specs

72911a9

Tensho force-pushed the extend-uri-specs branch from 752b4a2 to 0b8baa6 Compare December 11, 2017 10:58

Tensho commented Dec 11, 2017

View reviewed changes

Tensho force-pushed the extend-uri-specs branch from 0b8baa6 to 7fd0303 Compare December 11, 2017 11:17

michaelklishin requested changes Dec 11, 2017

View reviewed changes

Tensho added 2 commits December 11, 2017 17:50

Cleanup

dae58f0

Adjust settings description

5824697

new: - heartbeat - connection_timeout - channel_max - auth_mechanism - verify - fail_if_no_peer_cert - cacertfile - certfile - keyfile rename: timeout -> connection_timeout

Tensho force-pushed the extend-uri-specs branch from 7fd0303 to 94d22fb Compare December 11, 2017 15:51

Tensho commented Dec 11, 2017

View reviewed changes

Tensho force-pushed the extend-uri-specs branch from eb5e814 to 7daf2be Compare December 11, 2017 20:31

Tensho added 4 commits December 11, 2017 23:10

Parsing behavior for bare schema diverge depending of ruby stdlib ver…

5812f2c

…sion

Replace "escaped" with "%-encoded"

32f5a5c

Replace "fallbacks" to "falls back"

d19ec91

Rename "AMQP_PORTS" constant to AMQP_DEFAULT_PORTS and make it private

b35ec65

Tensho force-pushed the extend-uri-specs branch from 7daf2be to b35ec65 Compare December 11, 2017 21:10

michaelklishin approved these changes Dec 13, 2017

View reviewed changes

michaelklishin merged commit 645df33 into ruby-amqp:master Dec 13, 2017

Tensho deleted the extend-uri-specs branch December 13, 2017 16:49

This was referenced Dec 18, 2017

Connection Parameters Handling #70

Closed

Handle URI Query Parameters Tensho/bunny#1

Closed

Handle URI Query Parameters ruby-amqp/bunny#534

Merged

URI Query Parameters ruby-amqp/rubybunny.info#46

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend URI Parsing Specs #69

Extend URI Parsing Specs #69

Tensho commented Dec 11, 2017 •

edited

Loading

Tensho Dec 11, 2017 •

edited

Loading

michaelklishin Dec 11, 2017

Tensho Dec 11, 2017

michaelklishin Dec 11, 2017 •

edited

Loading

Tensho Dec 11, 2017

Tensho Dec 11, 2017

michaelklishin Dec 11, 2017

Tensho Dec 11, 2017

michaelklishin Dec 11, 2017

michaelklishin Dec 11, 2017

Tensho Dec 11, 2017

michaelklishin Dec 11, 2017

michaelklishin Dec 11, 2017

Tensho Dec 11, 2017

michaelklishin Dec 11, 2017

michaelklishin Dec 11, 2017

michaelklishin Dec 11, 2017

michaelklishin Dec 11, 2017

michaelklishin Dec 11, 2017

michaelklishin Dec 11, 2017

michaelklishin Dec 11, 2017

Tensho Dec 11, 2017

Tensho left a comment

michaelklishin commented Dec 11, 2017

Extend URI Parsing Specs #69

Extend URI Parsing Specs #69

Conversation

Tensho commented Dec 11, 2017 • edited Loading

Tensho Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelklishin Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tensho left a comment

Choose a reason for hiding this comment

michaelklishin commented Dec 11, 2017

Tensho commented Dec 11, 2017 •

edited

Loading

Tensho Dec 11, 2017 •

edited

Loading

michaelklishin Dec 11, 2017 •

edited

Loading