WIP: redo validation by content type #760

dtkav · 2018-11-08T00:00:21Z

This is a WIP, I'd like to make this a bit more pluggable, so that it is straightforward to add content-type validation handlers.

Related to #655 #755

Changes proposed in this pull request:

remove all_json requirement that caused any non-json content types to preclude validating json

hardbyte · 2018-11-09T05:08:30Z

connexion/decorators/validation.py

                if error:
                    return error

+            elif self.strict_validation:


If this is a global setting it means that if any endpoints accept non handled content types (e.g. binary uploads) then strict validation would have to be disabled for all endpoints.

Yeah - I'm still thinking through the implications. The strict flag seems pretty nebulous. AFAICT it mostly checks for extra form fields, and errors if those exist.

So far this diff makes connexion much more strict, which I personally think is a good thing, but others might disagree (and it may cause breakage for people that have been relying on connexion's past flexibility).

I think personally as a user I'd prefer to explicitly have a content handler for every content type that I accept. Part of my goal is to make it straightforward to register new content handlers.

We could also provide more 'batteries included' beyond just json and formdata. For example, I could add a default octet stream content handler.

Oh right I see. If it is straight forward to add a custom content handler that would be ideal for my case in #592 - especially if the custom handler/validator can process as a stream.

dtkav · 2018-11-15T04:22:03Z

hey @jmcs - I'd love your thoughts on this diff/demo.
This is a demo of a pluggable interface for handling different content-types.
While it is a fully working PR, I'm looking for concept and architectural level feedback right now.

kornicameister

Some minor comments along the way.

kornicameister · 2019-01-29T07:08:16Z

connexion/lifecycle.py

@@ -21,6 +20,7 @@ def __init__(self,
        self.json_getter = json_getter
        self.files = files
        self.context = context if context is not None else {}
+        self.content_type = self.headers.get("Content-Type", "")


If that's a derived thingy, perhaps a property and returning None if a content-type not set?

kornicameister · 2019-01-29T07:08:41Z

connexion/serialization/deserializers.py

+from ..types import coerce_type
+from ..utils import is_null
+
+logger = logging.getLogger('connexion.serialization.deserializers')


shouldn't __name__ be sufficient here?

I'm not sure why, but all of the other getLogger calls in connexion are hard-coded

If you don't mind, I might go through everything and clean it up for ya. Not sure when, but perhaps I can find some time today's evening or tomorrow.

kornicameister · 2019-02-03T16:37:16Z

connexion/content_types.py

+    )
+
+
+KNOWN_CONTENT_TYPES = [


Just a nit, you can define all constant lista as tuples. That way you limit the ammount of used memory + load time is a bit faster. Tuples are cached by python runtime and once defined they are kept as they were without a change.

I might skip creating KNOWN_CONTENT_TYPES and use ContentHandler.__subclasses__() instead. (see my comment on connexion/decorators/validation.py)

harph · 2019-09-02T17:31:27Z

Any updates on the status of this?

cognifloyd

Very nice direction. Lets finish it!

cognifloyd · 2019-12-07T02:26:46Z

connexion/content_types.py

+
+class JSONContentHandler(ContentHandler):
+    name = "application/json"
+    regex = re.compile(r'^application\/json.*|^.*\+json$')


is_json_mimetype checks for application/json or application/.*\+json but this checks for application/json or .*+json. They should probably both check for the same thing. So, maybe is_json_mimetype should do the equivalent of this regex. It makes sense to be even more lenient and treat anything that ends in +json as json.

There are several places that said validation can occur for text/plain as well as application/json - does this need to check for that too?

cognifloyd · 2019-12-07T02:36:00Z

connexion/content_types.py

+    )
+
+
+KNOWN_CONTENT_TYPES = [


I might skip creating KNOWN_CONTENT_TYPES and use ContentHandler.__subclasses__() instead. (see my comment on connexion/decorators/validation.py)

cognifloyd · 2019-12-07T02:38:52Z

connexion/decorators/validation.py

+            de(self.validator,
+               self.schema,
+               self.strict_validation,
+               self.is_null_value_valid) for de in KNOWN_CONTENT_TYPES


Suggested change

self.is_null_value_valid) for de in KNOWN_CONTENT_TYPES

self.is_null_value_valid) for de in ContentHandler.__subclasses__()

that way all you have to do to register a new one is subclass ContentHandler and make sure to import your ContentHandler subclass before initializing Connexion (or more accurately before whatever point it is that creates the RequestBodyValidator).

Actually, I think I would turn this into a classmethod on ContentHandler so that it can look for grandchildren not just children classes as described here: https://stackoverflow.com/a/3862957

Oh. I see register_content_handler below for registering additional content types. I would still like to see a way to replace default content handlers.

cognifloyd · 2019-12-07T02:57:25Z

connexion/decorators/validation.py

+        if len(matches) > 1:
+            logger.warning("Content could be handled by multiple validators")


Do we want them to be able to vote? ie more than just a regex? That way someone could register a handler that should handle (for example) json with a higher priority than the internal json validator.

One thing that the regex can't handle is None - what if a content handler wants to handle content_type = None (more likely to be used for serialization than deserialization/validation, assuming the same ContentHandlers are used for both processes).

cognifloyd · 2019-12-07T03:16:47Z

connexion/operations/abstract.py

+         - query params
+         - form params
+         - body params
+         - path params ?


What is the ? here?

connexion/operations/openapi.py

cognifloyd · 2019-12-07T04:05:03Z

connexion/content_types.py


-logger = logging.getLogger('connexion.serialization.deserializers')
+logger = logging.getLogger('connexion.content_types')


Couldn't we just do logging.getLogger(__name__) here (and everywhere else)? That's what I do in my other projects and it prevents pointless work when refactoring.

yeah, I think I was planning to stick to repo convention for now, and then doing a single PR to fix them all. Happy to do it incrementally though.

cognifloyd · 2019-12-07T04:14:13Z

tests/api/test_parameters.py

+    #time_start = 1010
+    #resp = app_client.get(
+    #    '/v1.0/nullable-parameters?time_start={}'.format(time_start))
+    #assert json.loads(resp.data.decode('utf-8', 'replace')) == time_start


Why did you comment this out? I see you did that with the empty_object_body test, but it looks like this was testing something different with parameters. Are you planning to adjust parameter validation to use ContentHandlers as well (that doesn't make complete sense to me)?

I'm not sure, but I think it was a mistake.

cognifloyd · 2019-12-07T04:38:04Z

connexion/operations/abstract.py

+        request comes in -> connexionrequest (framework generic)
+        API metrics recorded
+        API security applied
+
+        arrays are deserialized (spec dependent)
+         - query params
+         - form params
+         - path params
+
+        body is deserialized (spec dependent, content dependent)
+
+        validation is applied (spec dependent, content dependent)
+         - query params
+         - form params
+         - body params
+         - path params ?
+
+        parameter_to_arg (handler, what to call it)
+
+        json serializer is applied
+        response is validated if json


This is the plan, not what it currently does, right?

Mapping this out (current -> planned)

decorators.metrics -> API metrics

decorators.security -> API security (this is also spec dependent for oauth scopes and security schemes)

decorators.uri_parsing -> query & path params deserialiation

part of validation? -> form params

part of validation -> body deserialization

decorators.validation -> request validation: query, form, body, path

decorators.parameter -> parameter_to_arg (I would call this an argument mapper)

part of api -> response serializer (on content type handlers? Need a way to register custom like request validators)

part of validation -> response validation (on content type handlers? Need a way to register custom like request validators)

Is that what you're thinking?

cognifloyd · 2019-12-07T04:52:42Z

connexion/content_types.py

+    def deserialize(self, request):
+        return request.body
+
+    def validate(self, request):


Suggested change

def validate(self, request):

def validate_request(self, request):

Make the ContentHandlers useful in more contexts.

ie, we could add a validate_response_body() as well.

bilalshaikh42 · 2020-02-25T02:24:10Z

Hi @dtkav,
I was wondering what the status of this was and if there was a way to contribute?

AjaySharma2209 · 2020-06-02T00:29:41Z

Very basic question. So, in case if application/xml content type is used, is there any way of parsing xml at all. I am getting below error.
cannot convert dictionary update sequence element #0 to a sequence

Song2017 · 2022-03-10T02:24:41Z

Hey,
is it possible that request content only to be pass-through if content-type is not application/json, body content could be validated by developer self

RobbeSneyders · 2022-09-20T18:12:08Z

Closing this one as #1588 and #1591 tackle this for request and response validation respectively.

dtkav force-pushed the content_type_validation branch from e7938e5 to 2a50660 Compare November 8, 2018 02:25

spec-first deleted a comment Nov 8, 2018

dtkav mentioned this pull request Nov 8, 2018

Handling streaming request data #592

Closed

spec-first deleted a comment Nov 8, 2018

hardbyte reviewed Nov 9, 2018

View reviewed changes

dtkav changed the title ~~WIP: redo validation by content type~~ WIP DEMO: redo validation by content type Nov 9, 2018

dtkav force-pushed the content_type_validation branch 2 times, most recently from bd6d8a6 to de1a3df Compare November 9, 2018 06:21

spec-first deleted a comment Nov 9, 2018

spec-first deleted a comment Nov 10, 2018

dtkav force-pushed the content_type_validation branch from c7dfabf to d8429d6 Compare November 10, 2018 01:16

dtkav changed the title ~~WIP DEMO: redo validation by content type~~ DEMO: redo validation by content type Nov 10, 2018

spec-first deleted a comment Nov 10, 2018

spec-first deleted a comment Nov 13, 2018

dtkav force-pushed the content_type_validation branch from 29342ff to f87b84f Compare November 15, 2018 02:50

spec-first deleted a comment Nov 15, 2018

dtkav mentioned this pull request Nov 28, 2018

WIP: Fix validation of endpoint with multiple "produces" mimetypes. #652

Closed

spec-first deleted a comment Nov 28, 2018

Jyhess mentioned this pull request Dec 4, 2018

Python AIOHTTP server generator OpenAPITools/openapi-generator#1470

Merged

This was referenced Dec 15, 2018

Connexion 2.1.0 doesn't parse the request body using the right content types when more types are present #805

Closed

Validate Content Type #407

Closed

This was referenced Jan 8, 2019

Sanitization removes sensible characters in API definition and json request body input #835

Closed

Support for date and date-time type casting #344

Open

dtkav mentioned this pull request Jan 28, 2019

JSON request bodies are not strictly validated #837

Closed

kornicameister reviewed Jan 29, 2019

View reviewed changes

dtkav force-pushed the content_type_validation branch from cdc2091 to 650abaa Compare February 3, 2019 11:44

dtkav mentioned this pull request Feb 3, 2019

Content negotiation and response validation assumes incorrect schema for validation #860

Closed

kornicameister reviewed Feb 3, 2019

View reviewed changes

dtkav mentioned this pull request Nov 10, 2019

jsonify_data is called on ConnexionResponse regardless of mimetype/content_type #1062

Closed

This was referenced Dec 3, 2019

application/xml as requestbody does not work #1085

Closed

Return tuple for aiohttp #849

Merged

cognifloyd reviewed Dec 7, 2019

View reviewed changes

This was referenced Dec 7, 2019

New Branch for v2; Master for v3 #1096

Closed

Add plugins support #728

Closed

dtkav added 9 commits December 11, 2019 20:28

fixup recursive objects

53b74f6

fixup validation path error messages

452db0d

add empty-object test

bfa477b

fix register_content_handler

a95e987

tests pass but it's trash

062de8b

rename _deser method

5a028ee

begin to document and work out ideal decorator flow

2494326

rename from serialization/deserializers to content_types

fd89862

move request.content_type to property, with None as default

171cbb3

dtkav force-pushed the content_type_validation branch from 650abaa to 171cbb3 Compare December 12, 2019 04:47

dtkav added 2 commits December 11, 2019 23:01

content handler autodiscover

0023799

uncomment test

bfbb519

dtkav mentioned this pull request Dec 13, 2019

Switch to Calendar Versioning #1100

Closed

dtkav changed the title ~~DEMO: redo validation by content type~~ WIP: redo validation by content type Dec 13, 2019

wilko77 mentioned this pull request Feb 5, 2020

Feature openapi3 data61/anonlink-entity-service#479

Merged

m-nikhil mentioned this pull request Mar 25, 2020

pass correct content_type to is_json_schema_compatible #1191

Closed

AdriiiPRodri mentioned this pull request Jun 26, 2020

Penetration test for API 4.0 and RBAC wazuh/wazuh#5318

Closed

RobbeSneyders mentioned this pull request May 6, 2022

Make validation pluggable by content type #1528

Closed

RobbeSneyders closed this Sep 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: redo validation by content type #760

WIP: redo validation by content type #760

dtkav commented Nov 8, 2018

hardbyte Nov 9, 2018

dtkav Nov 9, 2018 •

edited

Loading

hardbyte Nov 9, 2018

dtkav commented Nov 15, 2018

kornicameister left a comment

kornicameister Jan 29, 2019

dtkav Feb 3, 2019

kornicameister Jan 29, 2019

dtkav Feb 3, 2019

kornicameister Feb 3, 2019

kornicameister Feb 3, 2019 •

edited

Loading

cognifloyd Dec 7, 2019

harph commented Sep 2, 2019

cognifloyd left a comment

cognifloyd Dec 7, 2019

cognifloyd Dec 7, 2019

cognifloyd Dec 7, 2019

cognifloyd Dec 7, 2019

cognifloyd Dec 7, 2019

dtkav Dec 7, 2019

cognifloyd Dec 7, 2019

cognifloyd Dec 7, 2019

cognifloyd Dec 7, 2019

cognifloyd Dec 7, 2019

dtkav Dec 12, 2019

cognifloyd Dec 7, 2019

dtkav Dec 12, 2019

cognifloyd Dec 7, 2019

cognifloyd Dec 7, 2019

dtkav Dec 12, 2019

bilalshaikh42 commented Feb 25, 2020

AjaySharma2209 commented Jun 2, 2020

Song2017 commented Mar 10, 2022

RobbeSneyders commented Sep 20, 2022

	self.is_null_value_valid) for de in KNOWN_CONTENT_TYPES
	self.is_null_value_valid) for de in ContentHandler.__subclasses__()

		if len(matches) > 1:
		logger.warning("Content could be handled by multiple validators")


		logger = logging.getLogger('connexion.serialization.deserializers')
		logger = logging.getLogger('connexion.content_types')

	def validate(self, request):
	def validate_request(self, request):

WIP: redo validation by content type #760

WIP: redo validation by content type #760

Conversation

dtkav commented Nov 8, 2018

Choose a reason for hiding this comment

dtkav Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtkav commented Nov 15, 2018

kornicameister left a 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

kornicameister Feb 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harph commented Sep 2, 2019

cognifloyd left a 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

bilalshaikh42 commented Feb 25, 2020

AjaySharma2209 commented Jun 2, 2020

Song2017 commented Mar 10, 2022

RobbeSneyders commented Sep 20, 2022

dtkav Nov 9, 2018 •

edited

Loading

kornicameister Feb 3, 2019 •

edited

Loading