Support all JSON configuration options #520

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

nichtich commented Jan 6, 2014

As noted in issue #512 the current JSON serializer ignored most JSON configuration options. This should fix it.

Support all JSON configuration options
As noted in issue #512 the current JSON serializer ignored most [JSON](https://metacpan.org/pod/JSON) configuration options. This should fix it.
Owner

veryrusty commented Jan 6, 2014

Thanks for the PR @nichtich.

Rather than explicitly listing all available config options (and having to update them as the JSON module changes), why not pass them all through to JSON? This is the approach that Dancer1 (now) takes; see this commit: PerlDancer/Dancer@e0d2b29

If you have the time, there may be other commits since that one that could be "forwarded ported" too (I haven't looked in great detail). And thanks again!

Contributor

nichtich commented Jan 15, 2014

By the way, UTF8 is broken anyway. The option ascii prevents emitting Unicode characters but it would be better to avoid double-encoding.

Contributor

nichtich commented Feb 4, 2014

Config options must be listed explicitly or any unknown config option will make the server die because the JSON module dies on unknown options.

Owner

veryrusty commented Feb 9, 2014

There are two (at least - this is perl..) ways to address this

  1. listing every JSON config option within the serializer, or
  2. remove the extra options that Dancer2 adds to its serialisation
    engine config before passing the rest to JSON.

I prefer the second option. If there is a change to the JSON API (excluding clashing of common configuration keywords), there will be is no change required to Dancer2's JSON serializer. Mistyped config entries also get picked up by a failure on first request (maybe I'm the only who has
dropped an 'r', ending up with petty => 1). As mentioned previously, this approach is then similar to the behaviour in Dancer1.

Owner

racke commented Feb 9, 2014

On 02/09/2014 08:23 AM, Russell Jenkins wrote:

There are two (at least - this is perl..) ways to address this

  1. listing every JSON config option within the serializer, or
  2. remove the extra options that Dancer2 adds to its serialisation
    engine config before passing the rest to JSON.

I prefer the second option. If there is a change to the JSON API (excluding clashing of common configuration keywords), there will be is no change required to Dancer2's JSON serializer. Mistyped config entries also get picked up by a failure on first request (maybe I'm the only who has
dropped an 'r', ending up with petty => 1). As mentioned previously, this approach is then similar to the behaviour in Dancer1.

Yes, I would also prefer the second option. Less maintenance and a crash on typos are good.

Regards
Racke

LinuXia Systems => http://www.linuxia.de/
Expert Interchange Consulting and System Administration
ICDEVGROUP => http://www.icdevgroup.org/
Interchange Development Team

Owner

veryrusty commented Jun 3, 2014

With db4ae15 removing the extra options from an engines' config that would cause JSON to die, passing all the engine config options through should be easy to implement.

@nichtich would you like to implement this? (Sorry for taking 5 months to get this far!!)

Owner

veryrusty commented Jun 3, 2014

Re-reading after a nights sleep; I see @nichtich did implement passing through all the engine configs (oops! 5 months ago again - Sorry!).

Owner

xsawyerx commented Jun 7, 2014

So... what would be the status of this?

Owner

veryrusty commented Jun 8, 2014

The config (as per comment on f7d8d22 lib/Dancer2/Serializer/JSON.pm:L28) still needs cleaning up. Some tests would be nice too.

As I'd like to see this resolved asap, I've done that cleanup and added tests in #602 to help speed up the review process :)

Owner

veryrusty commented Jun 14, 2014

Reworked into #602.

Merged as 4d97feb.

@veryrusty veryrusty closed this Jun 14, 2014

I have two questions about json serializer config and encoding.
Q1, how to set json config in app's config file(config.yml)?
I try to set json config as follows, but restart the app, it does not work.

Serializer:
JSON:
allow_blessed: 1
canonical: 1
utf8: 0


sub serialize {
my ( $self, $entity, $options ) = @_;

my $config = $self->config;

....
}

Dumper $config, it ouput:

$VAR1 = bless( {
             'content_type' => 'application/json',
             'config' => {},
             'hooks' => {
                          'engine.serializer.before' => [],
                          'engine.serializer.after' => []
                        }
           }, 'Dancer2::Serializer::JSON' );

Q2, the function to_json in Dancer2::Serializer::JSON module default enabled utf8

$options->{utf8} = 1 if !defined $options->{utf8};

if the entity is already utf8 encode, it will encode utf8 again, so the output json is broken.
I need to write code like this to_json {ok => 1, msg=> '中文' }, {utf8 => 0}, it can work but need to declare each time .
Dancer2 version is 0.150000.
Dancer2::Serializer::JSOn version is 0.150000.

Thanks.

DavsX commented Sep 22, 2014

Q2: We are aware of the problem (see #686). Right now passing {utf8 => 0} is the way to go, WHEN using to_json output as a response.

Q1: it should be set like this:

serializer: JSON
engine:
    serializer:
        JSON:
            allow_blessed: 1
            canonical: 0

Thank you, DavsX.
No problem with Q2.
For Q1 about config setting, I take your advice, setting in config.yml like this:

serializer: 'JSON'
engines:
    serializer:
        JSON:
            utf8: 0

The output of to_json is still broken if without {utf8 => 0}. Seems I still have problem with the config setting.
Dump Dancer2::Serializer::JSON::serrializer object, it output follows:

    $VAR1 = bless( {
                 'config' => {},
                 'content_type' => 'application/json',
                 'hooks' => {
                              'engine.serializer.after' => [],
                              'engine.serializer.before' => []
                            }
               }, 'Dancer2::Serializer::JSON' );

Dancer2 config dump as follows:

'engines' => {
                         'serializer' => {
                                           'JSON' => {
                                                     'utf8' => '0'
                                                   }
                                         },
                         'template' => {
                                       'template_toolkit' => {
                                                             'start_tag' => '[%',
                                                             'end_tag' => '%]'
                                                           }
                                     }
                       },
'serializer' => 'JSON'

DavsX commented Sep 22, 2014

Just guessing, but you could maybe try instead of:

serializer: 'JSON'
engines:
    serializer:
        JSON:
            utf8: 0

this

engines:
    serializer:
        JSON:
            utf8: 0
serializer: 'JSON'

I.e.: try setting the engine settings first, THEN the serializer

Owner

veryrusty commented Sep 22, 2014

@fangyuan @davx - there are some notes in #647 regarding why this doesn't work

Read both the notes in #647 and serializer source code, setting for json serializer in config.yml does not work. I know it. Thanks, @veryrusty .

DavsX commented Sep 22, 2014

oh..I tought that was already merged. btw @veryrusty DavX is someone else, not me ;)

Owner

veryrusty commented Sep 22, 2014

@DavsX - sorry!
/me mumbles about the lack of auto complete on githubs' mobile interface..

Owner

xsawyerx commented Sep 24, 2014

I really fucked up the Changelog then. :)

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