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

Only allow users to specify a single location per parse call & pass full location data to schema.load #420

Merged
merged 13 commits into from Jan 3, 2020

Commits on Nov 24, 2019

  1. Update changelog with change in location

    This is early documentation of planned changes, for this branch of work.
    
    Note the change from `locations=...` to `location` in
    Parser.use_[kw]args methods, and that fields may no longer specify a
    location. Additionally, detail that schemas now load all data from a
    location, not only those for which there are matching fields.
    
    The changelog explains how to rewrite a schema which loads from multiple
    locations into multiple `use_[kw]args` calls, and how to use `unknown`
    to get webargs v5-like behavior.
    sirosen committed Nov 24, 2019
    Configuration menu
    Copy the full SHA
    4099814 View commit details
    Browse the repository at this point in the history
  2. Start to implement marshmallow-code#419 (flaskparser first)

    Stated goals in the form of CHANGELOG update
    
    Gut significant chunks of webargs.core
    - replace
        "parse_{location}" with "load_{location}"
        "locations=<collection>" with "location=<string>"
        "location_handler" with "location_loader"
    - remove
        "parse_arg"
        "get_value"
    - add
        webargs.multidictproxy which uses a Schema to handle MultiDict types
        and behave similarly to webargs.core.get_value
    
    As a proving ground for this implementation/approach, get flaskparser
    tests to pass under ma3
    
    Fixes as the location loading code undergoes refactoring for the
    flaskparser:
    - Restore missing mimetype check for JSON payloads
    - Treat form data as a multidict without extraneous AttributeError check
      (after testing, flask will return '{}' for form data when it is
      missing or empty -- just respect/allow this)
    - req.headers is a multidict type as well
    - fix some mistakes with new argument ordering
    
    Changes to the base test app at src/webargs/testing.py of note:
    - Because each route can only specify that it loads data from a single
      location (unless you pull extra tricks), several testing cases have
      had their routes modified, e.g. "/echo" -> "/echo_json"
    - New test, test_parse_json_list_error_malformed_data added to check
      that webargs does not incorrectly treat non-list data as singleton
      lists when passing data to a schema with list fields. The previous
      behavior was questionable before, but with full data going to schemas
      it definitely becomes incorrect.
    - Many tests which previously expected success with malformed data
      ignored now expect 422 errors. For example, sending a non-dict JSON
      value (e.g. `1`) should fail because it cannot be loaded by the
      schema. It should *not* be silently ignored because data was provided
      and failed to parse. This is a major behavior change for webargs.
    
    Supporting modifications were made in tests/apps/flask_app.py ,
    including the addition of `ma.EXCLUDE` when the detected marshmallow
    version is >= 3.
    
    After work on the relevant PR and trying things both ways, the
    current handling of empty JSON bodies (interpreting them as {}) is being
    kept externally, but changed internally. This relates to the decision
    made in marshmallow-code#297 . Originally in the course of work on marshmallow-code#419, it seemed like
    it might be necessary to change the behavior to make missing JSON data
    behave as `missing` externally. However, making the change internal and
    wrapping it to present the same external interface is best.
    
    load_* methods are expected to return `missing` or {} when data is
    missing for the given location. e.g. When a JSON body is "" it should be
    `missing`. This is considered correct.
    However, in terms of the external behavior of webargs, no data should be
    interpreted as `{}`.
    
    In order to achieve this, the wrapper which calls the various
    "loader_func"s to load location data explicitly checks the result for
    `missing` and converts it to `{}` if it is missing.
    
    This makes it easy to use webargs to implement an API with optional JSON
    data, but internally preserves the ability to detect a missing JSON
    payload and distinguish it from a payload containing `{}`.
    sirosen committed Nov 24, 2019
    Configuration menu
    Copy the full SHA
    5a58a23 View commit details
    Browse the repository at this point in the history
  3. Update bottleparser, djangoparser for v6

    - Convert parse_* funcs to load_* funcs
    - Update test apps to work with testsuite changes
    - The django test app now decorates all view funcs so that validation
      errors get caught and translated to 422s (simplifies the test app)
    
    To support these changes, add a "concrete" implementation of `load_json`
    to the core parser.
    
    Because the whole point of the explicit error handling is to try to make
    the behavior of webargs more uniform, it makes sense to provide a couple
    of hook points for sending data into the main load_json method.
    The public interface is still the existence of load_json and users who
    want to customize parser behavior should simply override that. However,
    within webargs we can have some private hooks, `_raw_load_json` which
    doesn't deal with decode errors and `_handle_invalid_json_error` which
    processes errors.
    
    As a result, bottleparser, flaskparser, and djangoparser can all share
    code for this purpose. Presumably all of the other parsers will be able
    to share as well, but it remains to be seen.
    
    Per code review, update docstrings on all load_* methods returning
    MultiDictProxy objects in core and flaskparser as well.
    sirosen committed Nov 24, 2019
    Configuration menu
    Copy the full SHA
    70f3f90 View commit details
    Browse the repository at this point in the history
  4. Make tests explicit about ignoring extra data

    Some routes/methods are being tested to ensure that they accept
    additional data. In those cases and *only those cases*, the
    unknown=EXCLUDE argument should be passed to the schema when running
    under marshmallow 3.
    
    This schema must be used for handling headers as well, as otherwise
    normally seen headers like `Host` will cause schema validation errors.
    
    Additionally, fix an issue with the flaskparser under marshmallow2: the
    validation of a "many=True" schema against non-list data produced error
    messages in a different format from marshmallow 3 (use of `enumerate()`
    expanding out in a presumably-unexpected way). In order to compensate,
    add a check under marshmallow 2 for precisely this condition and
    reformat the error message in this case.
    As a result of this fix, tests are now passing for the flaskparser under
    all tox environs.
    
    Updates to bottleparser and djangoparser tests to match.
    
    This also reverts some changes to the test apps which turned out to be
    unnecessary.
    sirosen committed Nov 24, 2019
    Configuration menu
    Copy the full SHA
    f878d1c View commit details
    Browse the repository at this point in the history
  5. Update aiohttpparser for webargs v6

    - Convert "parse_*" methods to "load_*" style
    - Add needed methods to the testapp
    - Update aiohttp tests in minor ways to handle changes
    - Update asyncparser methods as well
    sirosen committed Nov 24, 2019
    Configuration menu
    Copy the full SHA
    6e41ef8 View commit details
    Browse the repository at this point in the history
  6. Convert remaining parsers for v6

    Updates falconparser, webapp2parser, tornadoparser, and pyramidparser.
    
    For the most part, this was a matter of
    - Convert parse_* to load_* methods, using MultiDictProxy
    - Update tests and test apps
    
    There are pecularities for Falcon and Tornado.
    
    - Some notes inline in falconparser about use of MultiDictProxy and
      handling of headers and cookies, as Falcon does not provide a
      high-level multidict representation of these things. (Webargs
      *could* provide one with great effort if it's ever wanted.)
    
    - In the case of Tornado, some custom multidict classes are needed to
      customize the handling of data.
    sirosen committed Nov 24, 2019
    Configuration menu
    Copy the full SHA
    b2ce557 View commit details
    Browse the repository at this point in the history
  7. Elaborate on core test for unknown=... behavior

    Rather than just comparing default schema behavior against
    unknown=EXCLUDE, compare IGNORE and RAISE as well.
    sirosen committed Nov 24, 2019
    Configuration menu
    Copy the full SHA
    f4d83b7 View commit details
    Browse the repository at this point in the history
  8. Ensure all parsers treat missing json equivalently

    If an empty body is submitted to a webargs parser, it should always be
    checked for content-type. Otherwise, the behavior framework-to-framework
    for webargs will not be consistent.
    
    Namely: djangoparser and pyramidparser now check content-type for JSON
    and return missing when an emtpy/missing payload is used. This makes the
    detection/use of JSON payloads more consistent and is necessary for
    `json_or_form`.
    sirosen committed Nov 24, 2019
    Configuration menu
    Copy the full SHA
    cf8bd16 View commit details
    Browse the repository at this point in the history
  9. Add the 'json_or_form' location + docs

    'json_or_form' is defined as first trying to load the JSON data, then
    falling back to form data.
    
    Adds a test to the testsuite for this which tries sending JSON, tries
    sending Form, and tries sending no data, verifying that we get the
    correct result in each case.
    
    This does *not* try to support more elaborate behaviors, e.g. multipart
    bodies, which probably will work variably well or poorly based on the
    framework in use. In these cases, the content type will not detect as
    JSON (because it is 'multipart'), and we'll failover to using form data.
    This is acceptable, since anyone using `json_or_form` is expecting to
    allow form posts anyway.
    
    Docs cover not only the new json_or_form location, but also the *idea*
    of a "meta location" which combines multiple locations. This is worth
    documenting in the advanced docs since it is the way to get back certain
    behaviors from webargs v5.x
    sirosen committed Nov 24, 2019
    Configuration menu
    Copy the full SHA
    071391b View commit details
    Browse the repository at this point in the history
  10. Rewrite narrative docs to be correct in v6

    This is not a deep and comprehensive rewrite which aims to
    discuss/explain the functionality which is now available. Rather, this
    change merely takes everything in the docs which has become inaccurate
    and trims or modifies it to be accurate again.
    
    Add notes to changelog about making parsers more consistent about
    checking Content-Type for JSON payloads.
    sirosen committed Nov 24, 2019
    Configuration menu
    Copy the full SHA
    6b92b2a View commit details
    Browse the repository at this point in the history

Commits on Dec 16, 2019

  1. Improvements per @lafrech's review

    1. Fix a typo in the quickstart doc
    
    Rewrite "content_type" to "user_type". Just a slip-up.
    
    2. Flatten out the nesting in MultiDictProxy.__getitem__
    
    Although this was flagged as an `else-return` which could flatten
    out to just a return, Jérôme also suggested a version which treats the
    outer condition as an `if ...: return` so that we can dedent the rest
    of the logic.
    The end-result is easier to read, so let's go with that.
    
    3. Add a test for passing a single value to a List field
    
    This applies to all parsers, but it was required by FalconParser in
    particular. If you remove MultiDictProxy from the presentation of
    query params, headers, etc. from FalconParser, it works in most cases.
    However, because field values only get wrapped in lists by that parser
    if there are multiple values supplied, we need some intermediary to
    understand that lookups for List fields always need a `list`-type
    value. MultiDictProxy serves that purpose, as a schema-aware dict type.
    sirosen committed Dec 16, 2019
    Configuration menu
    Copy the full SHA
    ae04c09 View commit details
    Browse the repository at this point in the history
  2. Switch JSON parsing to cache missing values

    Previously, `load_json` carefully avoided putting `missing` into the
    `Parser._cache["json"]` storage location. However, we're not clear (at
    least, in the context of marshmallow-code#420 ) on why we should avoid caching
    `missing` values. No tests fail, so change this to be simpler logic in
    which any "successful" JSON parsing, including those which failover to
    `missing`, will be cached.
    sirosen committed Dec 16, 2019
    Configuration menu
    Copy the full SHA
    4b12543 View commit details
    Browse the repository at this point in the history
  3. Fix Parser.parse docstring for 'location'

    Refer to the `__location_map__` attribute, and expand the list of
    supported locations to be complete (add `query` and `json_or_form`).
    sirosen committed Dec 16, 2019
    Configuration menu
    Copy the full SHA
    007ad84 View commit details
    Browse the repository at this point in the history