[manwar]

Summary

Resolves #1794. This PR introduces support for the HTTP QUERY method (draft-ietf-httpbis-safe-method-w-body), allowing application developers to define route handlers that accept complex, safe, body-based search parameters.

https://greenbytes.de/tech/specs/draft-ietf-httpbis-safe-method-w-body-latest.html

Key Changes

• Dancer2::Core::DSL: Added query keyword and route helper mapping.
• Dancer2::Core::Types: Updated %supported_http_methods allowlist to include QUERY.
• Dancer2::Core::Request: Updated deserialize() logic to automatically parse payload bodies for QUERY requests.
• Dancer2::Manual::Keywords.pod: Documented the query DSL keyword usage and semantics.
• New unit test in t/route_query.t covering route matching and body parsing

Testing Instructions

Run the test suite:

prove -lv t/

[bigpresh]

Firstly - many thanks for your contribution! If the HTTP QUERY method becomes widely used, we should definitely support it, and I appreciate you helping make that happen!

I will do a more detailed review soon, but my initial spots and knee-jerk "need to think about these" points:

• query is a fairly common name - if the app already has such a function, the query DSL keyword won't get installed as we silently skip DSL keywords if they are already defined...
  

  Dancer2/lib/Dancer2/Core/Role/DSL.pm

  Lines 52 to 58 in 90b6e65

  	foreach my $export ( keys %{$exports} ) {
  	no strict 'refs'; ## no critic (TestingAndDebugging::ProhibitNoStrict)
  	my $existing = *{"${caller}::${export}"}{CODE};
  	next if defined $existing;
  	*{"${caller}::${export}"} = $exports->{$export};

(should that bit of code emit a warning if a DSL keyword get skipped, to avoid surprises?) Something we probably want to think about in general, and may want to emphasise in release notes - if you already have a query sub defined you silently won't get Dancer2's, and if you try to define one after loading Dancer2 you'll start getting redefine warnings - obviously going to happen with any DSL addition, but something people upgrading to the next D2 version to carry this method need to consider

• $request->is_query() is documented, but not actually implemented

• any routes don't match query because the list of methods matched is hardcoded (mentioned in Handle shiny new QUERY method #1794)
  

  Dancer2/lib/Dancer2/Core/DSL.pm

  Lines 224 to 237 in 90b6e65

  	sub any {
  	my $self = shift;
  	# If they've supplied their own list of methods,
  	# expand del, otherwise give them the default list.
  	if ( is_arrayref($_[0]) ) {
  	s/^del$/delete/ for @{ $_[0] };
  	}
  	else {
  	unshift @_, [qw/delete get head options patch post put/];
  	}
  	$self->_normalize_route(@_);
  	}

  
  There probably ought to be one source of "these are the HTTP verbs".

t/http_methods.t doesn't test query (and again has a hardcoded list of methods which should probably be pulled from one canonical source so that all methods we claim to support are tested...)

• Terribly minor but keyword ordering: query sits between patch and path in dsl_keywords (lib/Dancer2/Core/DSL.pm:88-89); the hash is otherwise alphabetical, so it belongs after prefix.

[manwar]

@bigpresh Thanks for quick review. I will go through each point you raised and get back to you.

[manwar]

@bigpresh, I have updated the PR to address all issues raised so far:

1. Implemented $request->is_query: Added sub is_query to Dancer2::Core::Request.
2. any route support: Added query to the default method list in Dancer2::Core::DSL::any.
3. t/http_methods.t: Added query to the test suite matrix.
4. Keyword ordering: Moved query in dsl_keywords so it is ordered alphabetically after prefix.

Regarding the observation about app functions silently overriding DSL keywords during export (export_symbols_to):
since that behaviour applies globally to all DSL keywords in Dancer2::Core::Role::DSL, it would be worth discussing
and handling under a separate issue/PR to determine if we want to emit warnings or adopt a deprecation strategy core-wide.

[davorg]

My PR from yesterday addresses the is_query() and any problems.

manwar#1

[cromedome]

@manwar Nice work! Looks like you took care of most all of @bigpresh 's findings. I'll create a separate ticket for the "single source of truth" for supported HTTP verbs.

Just to spell out the potential breakage by the query keyword:

• Old-style plugins that do register query => ... now die at load time with "You can't use 'query', this is a reserved keyword." This is a hard break. I couldn't confirm whether any CPAN plugins hit it: my CPAN grep came back empty, but the search wasn't reliable enough to trust. Private plugins are the bigger unknown anyway.
• Apps with their own sub query still work, but now emit "Subroutine query redefined." Under fatal warnings, they die.
• New-style plugins exporting query still work, with an "Overwriting existing sub" warning.

These can be covered in the release announcement/Changes/similar. I just don't want them to get lost :)

Two other minor things I found:

• Broken doc link. Keywords.pod links to Dancer2::Manual/query, which doesn't exist. Its neighbors link to "HTTP Methods: Visitor Actions". (The existing post entry has the same broken link.)
• Manual not updated. That "HTTP Methods: Visitor Actions" section in Manual.pod doesn't mention QUERY at all.

Thanks for doing this! Looking forward to merging it.

[manwar]

@cromedome thanks for review. I have fixed the broken link and updated pod as you correctly pointed out.

[SysPete]

Great addition, thanks @manwar (and @davorg for the fix PR). I've added a few minor comments, but I had to be really picky. 😁

[manwar]

Thanks @SysPete. I have pushed the changes as per your suggestions. Please review when you get time.

[SysPete]

Thank you @manwar 👍

[cromedome]

👍 thanks, @manwar! Merging.

[manwar]

@cromedome @bigpresh @SysPete Thank you.