Remove Cornice default json renderer?

[wrkhenddher]

Hi folks,

I came across this pyramid feature that allows to serialize anything into JSON but also found this cornice PR comment that says it's not fully functional.

What's the current state of affairs of a custom/overloaded renderer?

Ideally, one could bypass the colander serializer and pyramid renderer all together, even return the JSON string straight from PostgreSQL JSONB column, perhaps?

cornice/cornice/util.py

Lines 51 to 71 in 66ce29f

	# XXX Patched with ``simplejson.dumps(..., use-decimal=True)``
	# if the renderer has been configured to serialise using just
	# ``json.dumps(...)``. This maintains backwards compatibility
	# with the Cornice renderer, whilst allowing Pyramid renderer
	# configuration via ``add_adapter`` calls, at the price of
	# rather fragile patching of instance properties.
	if renderer_factory.serializer == json.dumps:
	renderer_factory.serializer = simplejson.dumps
	if 'use_decimal' not in renderer_factory.kw:
	renderer_factory.kw['use_decimal'] = True
	renderer = renderer_factory(None)
	# XXX This call has the side effect of potentially setting the
	# ``response.content_type``.
	json_str = renderer(data, context)
	# XXX So we (re)set it ourselves here, i.e.: *after* the previous call.
	content_type = (request.accept.best_match(self.acceptable) or
	self.acceptable[0])
	response.content_type = content_type
	return json_str

[leplatrem]

To be honest, I can't recall much about that.

If we can, I would really be in favor of removing anything specific we have in the code base.
You can try to get rid of the lines you highlighted and see which tests fail.

This one caught my attention for example:

cornice/tests/test_validation.py

Lines 121 to 122 in 31bbdb9

	response = app.put('/service3', headers={'Accept': 'text/*'})
	self.assertEqual(response.content_type, "text/json")

If you investigate about this, please share your findings ;)

[wrkhenddher]

Thanks @leplatrem

Sounds like a plan - I'll comment here whatever I find out.

The code you cited seems interesting as it's not using the official internet media type 'application/json' (https://www.iana.org/assignments/media-types/media-types.xhtml#text) ... I also wonder what might break if we change that.

Thanks!

[leplatrem]

The code you cited seems interesting as it's not using the official internet media type 'application/json' (https://www.iana.org/assignments/media-types/media-types.xhtml#text) ... I also wonder what might break if we change that.

Also, I wouldn't mind releasing a new major version with a breaking change about that :)

[wrkhenddher]

Sorry for the loooooong delay @leplatrem

Turned out that we didn't have to touch Cornice's JSON renderer at all. Instead, serializing our domain objects to a dict that only had JSON compatible types within was enough. Basically, that dict returned from our cornice.Service subclass could be serialized by Cornice's JSON renderer without any problem as expected.

In our case, we added a Postgres JSON(B) column to our domain objects and such when read from DB by SQLAlchemy is just a vanilla dict that can be returned by the service. No need to return a custom pyramid Response object or anything special.

It would be ideal to understand if the existence of the custom Cornice's JSON renderer is truly needed. But, further investigation would be required.

For our practical purposes, the custom Cornice's JSON renderer was irrelevant in our case so this issue doesn't apply to us anymore (and may be closed).

I did though remove all instances of text/json (See #478)

There were few changes related to #113 that @msabramo identified back when.

[leplatrem]

In my opinion, less code is better. We should get rid of it :)

Note: This part was written 7 years ago

[devilicecream]

Hey @leplatrem I found this which is renderer related. It's easily fixable now removing the content-type overriding logic of Cornice. I'm hesitant to do it though, cause I don't know why that logic was there in the first place, and I think it would be a breaking change. What do you think it's best to do?

[leplatrem]

Now that we have this overriding logics I think that this is less urgent. We could add a deprecation warning, and remove it in the future when we release another major version with breaking changes. The current master has enough I believe