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

Add ability to transform query params on redirector services #3118

Closed
calebcartwright opened this issue Feb 28, 2019 · 4 comments
Closed

Add ability to transform query params on redirector services #3118

calebcartwright opened this issue Feb 28, 2019 · 4 comments
Assignees
Labels
core Server, BaseService, GitHub auth, Shared helpers

Comments

@calebcartwright
Copy link
Member

Dialog in #3074, in particular #3074 (comment)

Use case/benefit is being able to simplify route patterns by moving certain optional params (for example tokens, urls, etc.) from route to query params

@calebcartwright calebcartwright self-assigned this Feb 28, 2019
@paulmelnikow paulmelnikow added the core Server, BaseService, GitHub auth, Shared helpers label Feb 28, 2019
@calebcartwright
Copy link
Member Author

calebcartwright commented Mar 1, 2019

So I was originally only thinking about this in the context of a redirect being able to transform a route param to a query param, but as I've started working on this I realized that there may be a scenario when one would want to rename a query param (as part of a redirect, or even just in general)

My current thinking/assumption: #3042 added functionality that allows a service to specify query params via a Joi schema, and therefore renaming of a query param (?foo=... --> ?bar=...) would best be handled via Joi's rename functionality in the service's query param schema.

Accordingly, I'm not currently planning on adding functionality to the redirector for renaming/transforming one query param to another query param.

Please let me know if I've got anything wrong there and/or if anyone feels differently

@paulmelnikow
Copy link
Member

Redirecting is a bit different from renaming. Redirecting is more cache-friendly. It's nice for the user, and I think less confusing, to canonicalize the query params as we're rewriting the URL, particularly the badge-specific query params. Though that probably is relatively low priority.

As I think about it, the more important case of redirecting the query params would be in the context of an existing service (i.e. where the route is the same, but the query params are rewritten). So that feels like a different project entirely.

@calebcartwright
Copy link
Member Author

So that feels like a different project entirely.

So agreement that it's sufficient for the redirector transformQueryParams function to only allow for converting a route/pattern param to a query param as part of the redirect?

I.e. redirect: /codecov/c/token/abc123def456/github/foo/bar.svg -> /codecov/c/github/foo/bar.svg?token=abc123def456

@paulmelnikow
Copy link
Member

Yep!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Server, BaseService, GitHub auth, Shared helpers
Projects
None yet
Development

No branches or pull requests

2 participants