Skip to content

Conversation

@dreispt
Copy link
Member

@dreispt dreispt commented Dec 23, 2024

No description provided.

@OCA-git-bot
Copy link
Contributor

Hi @simahawk,
some modules you are maintaining are being modified, check this out!

@dreispt dreispt requested a review from simahawk December 23, 2024 11:49
@dreispt dreispt changed the title [IMP] endpoint: support usage of querystring parameters [14.0][IMP] endpoint: support usage of querystring parameters Dec 23, 2024
@dreispt
Copy link
Member Author

dreispt commented Dec 27, 2024

Review is appreciated but please don't merge yet, there might be a few changes.

@dreispt dreispt marked this pull request as draft December 27, 2024 13:17
@dreispt dreispt force-pushed the 14-dr-endpoint-params branch from d1bff46 to 3532d63 Compare January 2, 2025 12:29
@dreispt dreispt marked this pull request as ready for review January 3, 2025 10:18
@dreispt
Copy link
Member Author

dreispt commented Jan 3, 2025

Ready for review.

Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Something I was thinking about since a while ;)

Just 1 remark about the naming: I prefer to make explicit everywhere down the stack, as params is quite an abused term 😉

Also, would be nice to see a test.

* Response
* werkzeug
* exceptions
* params
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* params
* querystring_params

)

def _handle_exec__code(self, request):
def _handle_exec__code(self, request, params=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def _handle_exec__code(self, request, params=None):
def _handle_exec__code(self, request, querystring_params=None):

raise NotFound()
endpoint._validate_request(request)
result = endpoint._handle_request(request)
result = endpoint._handle_request(request, params=params)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
result = endpoint._handle_request(request, params=params)
result = endpoint._handle_request(request, querystring_params=params)

if not self._code_snippet_valued():
return {}
eval_ctx = self._get_code_snippet_eval_context(request)
eval_ctx["params"] = params or {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
eval_ctx["params"] = params or {}
eval_ctx["querystring_params"] = querystring_params or {}

)

def _handle_request(self, request):
def _handle_request(self, request, params=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def _handle_request(self, request, params=None):
def _handle_request(self, request, querystring_params=None):

handler = self_with_user._get_handler()
try:
res = handler(request)
# In case the handler does not support params
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt we have any other handler ATM but just in case... ok, let's keep this but I would add a separated commit for backward compat that we can easily drop on migration.
Something like:

[imp] endpoint: support querystring params
[imp] endpoint: backward compat for querystring params

Old handlers might not accept this new kw args
Trash this commit in future versions. Eg: v18.

Comment on lines +198 to +199
if params:
res = handler(request, params=params)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if params:
res = handler(request, params=params)
if querystring_params:
res = handler(request, querystring_params=querystring_params)

@dreispt dreispt force-pushed the 14-dr-endpoint-params branch from 3532d63 to f002179 Compare March 16, 2025 19:20
@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jul 20, 2025
@dreispt dreispt removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jul 21, 2025
@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Nov 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale PR/Issue without recent activity, it'll be soon closed automatically.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants