Skip to content

Conversation

@girijaasoni
Copy link
Contributor

No description provided.

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Do we really need this? On latest hammer-cli and hammer-cli-foreman I get:

Screenshot-1739795257755

UPD: We could rather treat this PR for #636 (comment)

@girijaasoni
Copy link
Contributor Author

Do we really need this?

Yes, because when passing the search params from CLI, we don't need to URL encode the search query. We need to do that only when using curl command

I can add tests too in a new ref PR

@ofedoren ofedoren changed the title Correct desciption Refs #38124 - Add correct description for invalidating multiple users jwt Feb 17, 2025
@ofedoren
Copy link
Member

Yes, because when passing the search params from CLI, we don't need to URL encode the search query. We need to do that only when using curl command

I'd say we can then remove URL-encoded from API description as well. curl can be used in a more user friendly way without a need to encode anything: curl -u user:password -H 'Content-Type:application/json' -X DELETE https://foreman.ofedoren.com/api/v2/registration_tokens -d '{ "search": "id ^ (5)" }'

I can add tests too in a new ref PR

👍

@girijaasoni
Copy link
Contributor Author

girijaasoni commented Feb 20, 2025

without a need to encode anything: curl -u user:password -H 'Content-Type:application/json' -X DELETE https://foreman.ofedoren.com/api/v2/registration_tokens -d '{ "search": "id ^ (5)" }'

We are sending search params as a part of url parameters. We need to URL encode it there
it is something like curl --user user:password -X DELETE http://localhost:3000/api/registration_tokens?search=id%20%5E%20%281%2C2%2C3%29 -H 'Content-Type: application/json

@ofedoren
Copy link
Member

We are sending search params as a part of url parameters. We need to URL encode it there it is something like curl --user user:password -X DELETE http://localhost:3000/api/registration_tokens?search=id%20%5E%20%281%2C2%2C3%29 -H 'Content-Type: application/json

Who are we and why do we do that? I mean, the API can be used that way, sure. But that's just a curl example, which can accept both ways (your and mine) to pass the parameters. Most tools/libs will just do that for you.

Wording URL-encoded even in API is kinda misleading since it forces one of the variants. Moreover, we don't do that for other search string params:

Screenshot-1740049729399
Screenshot-1740049743796

@girijaasoni
Copy link
Contributor Author

We are sending search params as a part of url parameters. We need to URL encode it there it is something like curl --user user:password -X DELETE http://localhost:3000/api/registration_tokens?search=id%20%5E%20%281%2C2%2C3%29 -H 'Content-Type: application/json

Who are we and why do we do that? I mean, the API can be used that way, sure. But that's just a curl example, which can accept both ways (your and mine) to pass the parameters. Most tools/libs will just do that for you.

@ShimShtein , what do you think about this?

@girijaasoni
Copy link
Contributor Author

converting this PR to draft. will be using it later for adding hammer tests.

@girijaasoni girijaasoni marked this pull request as draft February 27, 2025 10:31
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

Successfully merging this pull request may close these issues.

2 participants