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

route_param regression from 0.86 -> 0.87 #84

Open
djzort opened this issue Oct 8, 2019 · 9 comments
Open

route_param regression from 0.86 -> 0.87 #84

djzort opened this issue Oct 8, 2019 · 9 comments

Comments

@djzort
Copy link
Contributor

djzort commented Oct 8, 2019

This works in 0.86 but not in 0.87

resource 'etd' => sub {

    summary 'Get an estimate';
    desc 'Estimated Time of Delivery';
    route_param 'locale' => sub {
        route_param 'postcode' => sub {

            # query param style
            before_validation \&fix_material;
            params(
                requires( 'locale',   type => Locale,   desc => 'Locale (AU/NZ)' ),
                requires( 'postcode', type => PostCode, desc => 'Post Code' ),
                requires(
                    'material',
                    type => ArrayRef [MaterialCode],
                    desc => 'Material Code(s)'
                ),
                optional(
                    'orderdate',
                    type    => Str,
                    desc    => 'Order Date',
                    default => 'now'
                )
            );
            summary
              'GET one or more products via material code as query parameter';
            get \&guess_etd;

            # url path style
            params(
                requires( 'locale',   type => Locale,   desc => 'Locale (AU/NZ)' ),
                requires( 'postcode', type => PostCode, desc => 'Post Code' ),
                requires(
                    'material',
                    type => MaterialCode,
                    desc => 'Material Code'
                ),
                optional(
                    'orderdate',
                    type    => Str,
                    desc    => 'Order Date',
                    default => 'now'
                )
            );

            route_param 'material' => sub {
                summary 'GET a single product via material code in url';
                get \&guess_etd;
            };    # route_param 'material'

        };    # route_param 'postcode'
    };    # route_param 'locale'

};    # resource 'etd'

The api is as follows

/etd///

or

/etd///?material=&material=

Locale, PostCode, ad MaterialCode are custom constraints.

What doesnt work in 0.87 is the route_param version. The &guess_etd is called BUT the $_[0] is empty rather than containing the params as it does with the material in query query version

the fix_material sub massages the material= so that its always an arrayref even if there is just one value (which is an annoying bug of itself, as a single value will give a scalar and the type will fail)

@khrt
Copy link
Owner

khrt commented Oct 8, 2019

I'll take a look ASAP. Thanks for reporting.

@mschout
Copy link
Contributor

mschout commented Oct 9, 2019

Maybe I misunderstand how route_param is supposed to be used, but aren't you supposed to pair route_param with a params() call describing the route param like this?

params( requires('locale', type => Locale, desc => '...') )
route_param locale => sub {
   ...
};

@djzort
Copy link
Contributor Author

djzort commented Oct 10, 2019

To me, its not clear how route_params can be mixed with query params... but saying that the above code works as described in 0.86 and doesnt in 0.87. But strangely, it routes correctly in 0.87 but doesnt populate the $_[0] with a hashref of parameters.

If i have misinterpreted the documentation and somehow relied upon non-intentional behaviour i am happy to change my code. But for now i have Raisin pinned to 0.86 in my cpanfile

@mschout
Copy link
Contributor

mschout commented Oct 10, 2019

Part of the problem is that nested route_param was broken badly in 0.86 and prior, in that the inner route_param would blow away all param definitions of previous route_param declarations.

This was fixed in 0.87.

I suspect this is what you need for 0.87:

resource 'etd' => sub {
    summary 'Get an estimate';
    desc 'Estimated Time of Delivery';
    params( requires( 'locale',  type => Locale, desc => 'Locale (AU/NZ)' ) );
    route_param 'locale' => sub {
        params( requires( 'postcode', type => PostCode, desc => 'Post Code' ) );
        route_param 'postcode' => sub {
           ...
            # query param style
            before_validation \&fix_material;
            params(
                requires( 'material', ... );
                optional( 'orderdate', ... );
            );
            summary 'GET one or more products via material code as query parameter';
            get \&guess_etd;

            # url path style
            params( requires( 'material', ... ) );
            route_param 'material' => sub {
                summary 'GET a single product via material code in url';
                params( optional( 'orderdate', ... ) ):
                get \&guess_etd;
            };    # route_param 'material'
        };    # route_param 'postcode'
    };    # route_param 'locale'
};    # resource 'etd'

And as a nice benefit you don't have to repeat the postcode and locale params for every internal endpoint.

But per the raisin docs, preceding the route_param with a params() call that describes the route param seems to be the recommended way to deal with route params.

@djzort
Copy link
Contributor Author

djzort commented Nov 25, 2019

Ok this seems to work but has the annoying side effect that previously if the route_param value type didnt match the type it would return 400. Where as now it returns a 404. I am guessing this is due to failing to match causing it to not route past the handler code.

@khrt
Copy link
Owner

khrt commented Nov 27, 2019

That indeed is not fixed yet. It's on my roadmap.

@matya
Copy link

matya commented Oct 7, 2020

Just out of curiosity, because I did not find it anywhere, and before digging into the code, I would ask: How do you access the postcode and locale variables in guess_etd?
From what I see while playing around with it is that the get sub's first param is only returning the params on the level it is defined...

@khrt
Copy link
Owner

khrt commented Oct 7, 2020

@matya,

that would be something like:

get sub {
        my $params = shift;
        $params->{postcode};
        $params->{locale};
        $params->{...};
}

Where $params is a hashref to all the parameters parsed by:

sub prepare_params {

The problem here must be that nested path arguments are not stored properly and/or got overwritten somewhere inside that function:

%SETTINGS = %{ merge(\%SETTINGS, \%args) };

This would be the patch which introduced the issue: 06d50a9

@matya
Copy link

matya commented Oct 7, 2020

my bad, I messed up the params block. thanks!
I have found some examples I was looking for in the test cases (which I didn't think of looking at).

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

No branches or pull requests

4 participants