Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Request params parser #9

Merged
merged 13 commits into from Sep 24, 2012

Conversation

Projects
None yet
2 participants
Owner

myronmarston commented Sep 21, 2012

This is a new interpol tool that does two things:

  • Validates request params according to a schema definition.
  • Parses request params based on a schema definition.

Can I get a code review, @proby? /cc @waltjones @tithonium @dudleycarr

@proby proby commented on the diff Sep 24, 2012

lib/interpol/request_params_parser.rb
+ def no_additional_properties?
+ [
+ @endpoint_definition.path_params,
+ @endpoint_definition.query_params
+ ].none? { |params| params['additionalProperties'] }
+ end
+
+ STRING_EQUIVALENTS = {
+ 'string' => nil,
+ 'integer' => { 'type' => 'string', 'pattern' => '^\-?\d+$' },
+ 'number' => { 'type' => 'string', 'pattern' => '^\-?\d+(\.\d+)?$' },
+ 'boolean' => { 'type' => 'string', 'enum' => %w[ true false ] },
+ 'null' => { 'type' => 'string', 'enum' => [''] }
+ }
+
+ def adjusted_schema(schema)
@proby

proby Sep 24, 2012

This method name + parameter seems a little clumsy. I don't have a better suggestion off the top of my head though.

@myronmarston

myronmarston Sep 24, 2012

Owner

I agree but don't have any better ideas.

@proby proby commented on the diff Sep 24, 2012

lib/interpol/request_params_parser.rb
+
+ def validate!(params)
+ @validator.validate!(params)
+ end
+
+ # Private: This takes care of the validation.
+ class ParamValidator
+ def initialize(endpoint_definition)
+ @endpoint_definition = endpoint_definition
+ @params_schema = build_params_schema
+ end
+
+ def validate_path_params_valid_for_route!
+ route = @endpoint_definition.route
+ invalid_params = property_defs_from(:path_params).keys.reject do |param|
+ route =~ %r</:#{Regexp.escape(param)}(/|$)>
@proby

proby Sep 24, 2012

It's initially confusing what this does.

@myronmarston

myronmarston Sep 24, 2012

Owner

This looks at your route definition and validates that every defined path_param is actually a :blah param in the route string. Maybe it's not needed but I figured it was good to verify that the path_params you have defined are in fact part of the routing path.

@proby proby commented on the diff Sep 24, 2012

lib/interpol/request_params_parser.rb
+ end
+ end
+
+ NULLS = { '' => nil, nil => nil }
+ def self.convert_null(value)
+ NULLS.fetch(value) do
+ raise ArgumentError, "#{value} is not convertable to null"
+ end
+ end
+
+ def self.convert_date(value)
+ unless value =~ /\A\d{4}\-\d{2}\-\d{2}\z/
+ raise ArgumentError, "Not in iso8601 format"
+ end
+
+ Date.new(*value.split('-').map(&:to_i))
@proby

proby Sep 24, 2012

Why did you use this instead of Date.parse(value)?

@myronmarston

myronmarston Sep 24, 2012

Owner

Historically, Date.parse has been quite slow. I think it's a lot better in 1.9.3 than prior versions of ruby (I think I heard it's in C now), but this should still be faster, I think. Date.parse handles many different date formats, and it has to be tolerant of you using any of them. Here we know that we have 3 integers, which is exactly what the normal constructor needs, so we can bypass the complicated parsing logic entirely.

@myronmarston

myronmarston Sep 24, 2012

Owner

BTW, I originally used Date.iso8601(value) but that doesn't work on 1.8.7.

@proby proby commented on the diff Sep 24, 2012

lib/interpol/sinatra/request_params_parser.rb
+ available_versions ||= endpoint.available_versions
+ interpol_config.api_version_for(env, endpoint).tap do |_version|
+ version ||= _version
+ end
+ end
+
+ if definition == DefinitionFinder::NoDefinitionFound
+ interpol_config.request_version_unavailable(self, version, available_versions)
+ end
+
+ definition
+ end
+ end
+
+ def params
+ @_parsed_params || super
@proby

proby Sep 24, 2012

Why did you add the first underscore to this variable name?

@myronmarston

myronmarston Sep 24, 2012

Owner

These instance variables live in the same space as the instance variables in an end-user's Sinatra application. I used the underscore prefix to minimize the likelihood of a naming collision.

@myronmarston myronmarston merged commit f8605e9 into master Sep 24, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment