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

Add more flexible parsing #60

Merged
merged 1 commit into from
Jul 24, 2014
Merged

Add more flexible parsing #60

merged 1 commit into from
Jul 24, 2014

Conversation

stephencelis
Copy link
Member

As it stands, Parser::Simple is, on its own, too simple.

While Endpoint splits responsibility between acceptance and rendering, Endpoint does no such thing between supported media types and parsing.

Why do we need granularity here? Well, the parser supports application/x-www-form-urlencoded and multipart/form-data by default. This seemed like a good idea at a time, but it actually makes Crepe a bit more difficult to step into because the first time you curl-debug, the params you expect to be parsed in from JSON will be nil unless you explicitly add -H "Content-Type: application/json". If it returned early with 415 Unsupported Media Type, it would be a lot easier to debug.

Renderer::Simple supports to_#{format} for any format by default, so we shouldn't need to get rid of form data support, but Renderer::Simple won't get hit at all if the format hasn't been whitelisted. We need the same ability to whitelist supported media types.

This commit adds and configures a list of media types an endpoint will parse. In order to parse form data, you must be explicit with the media type, e.g.:

parse 'application/x-www-form-urlencoded', 'multipart/form-data'
# or parse(*Rack::Request::FORM_DATA_MEDIA_TYPES)

@stephencelis stephencelis mentioned this pull request Jul 22, 2014
@stephencelis
Copy link
Member Author

I'm not sure this is ready. The naming still feels muddled to me. Thoughts?

Also, it seems to me that by no longer requiring the with: key (to fall back to the default Parser) probably should raise an error in Parser::Simple if there is no match. Should it raise an exception and trigger a 500?

@stephencelis
Copy link
Member Author

Rebased, added an error and specs. Ready to merge when you are, @kainosnoema and @davidcelis.

@davidcelis
Copy link
Member

👍

media_types = Util.media_types media_types
media_types.each { |t| config[:parsers][t] = parser }
def parse *media_types, with: config[:parsers].default
config[:parses].concat media_types
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we uniq! this array?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or a Set, if that makes sense? Might be slightly more performant at runtime?

Kind of an open question, though. .respond_to actually completely resets the state of things (because when you're in a scope for a CSV-only endpoint, you don't want to look up the config stack and also respond with JSON—come to think of it, this may be a bug, because in what format do you return errors if you don't re-respond_to :json in a scope?). Maybe this applies to parsers, as well?

Alternatively, we should move to a situation where we always just merge config, but document how to clean that state out?

@kainosnoema
Copy link
Contributor

So we no longer parse form data implicitly? I suppose that makes sense, but it's a little confusing that we can technically parse it, just choose not to unless you add the media-type.

Otherwise, 👍

@stephencelis
Copy link
Member Author

it's a little confusing that we can technically parse it, just choose not to unless you add the media-type.

Not too far off from the fact that we to_#{format} anything that responds to it in Renderer::Simple, but choose not to unless you explicitly respond to the format.

@stephencelis
Copy link
Member Author

@kainosnoema Any specific changes we should make now?

@stephencelis
Copy link
Member Author

This still actually needs a little work, come to think of it. My proposal: two methods that work kind of like .respond_to and .render: .parses and .parse.

class MyAPI < Crepe::API
  # .parse is used to assign parser classes to media types, but doesn't change
  # what is supported
  parse :csv, with: CSVParser

  namespace :users do
    namespace :import do
      # .parses changes what's supported in the namespace
      #
      # Shorthand for doing both at once:
      #   parses csv: CSVParser
      parses :csv
      post { ... }
    end
  end
end

The methods are really similarly-named, though, so I'm up for suggestions of changing .parses to something like .supports, but less genetic.

As it stands, Parser::Simple is, on its own, too simple.

While Endpoint splits responsibility between acceptance and rendering,
Endpoint does no such thing between supported media types and parsing.

Why do we need granularity here? Well, the parser supports
`application/x-www-form-urlencoded` and `multipart/form-data` by
default. This seemed like a good idea at a time, but it actually makes
Crepe a bit more difficult to step into because the first time you
`curl`-debug, the params you expect to be parsed in from JSON will be
`nil` unless you explicitly add `-H "Content-Type: application/json"`.
If it returned early with `415 Unsupported Media Type`, it would be a
lot easier to debug.

Renderer::Simple supports `to_#{format}` for any format by default, so
we shouldn't need to get rid of form data support, but Renderer::Simple
won't get hit at all if the format hasn't been whitelisted. We need the
same ability to whitelist supported media types.

This commit adds and configures a list of media types an endpoint will
parse, with `.parses`, which is basically `.respond_to` for request
bodies.  In order to parse form data, you must be explicit with the
media type, e.g.:

  parses 'application/x-www-form-urlencoded', 'multipart/form-data'
  # or parses(*Rack::Request::FORM_DATA_MEDIA_TYPES)

New parsers are still registered with `.parse`.

Signed-off-by: Stephen Celis <stephen@stephencelis.com>
stephencelis added a commit that referenced this pull request Jul 24, 2014
@stephencelis stephencelis merged commit ee059bb into master Jul 24, 2014
@stephencelis stephencelis deleted the flexible-parsing branch July 24, 2014 15:19
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

Successfully merging this pull request may close these issues.

None yet

3 participants