-
-
Notifications
You must be signed in to change notification settings - Fork 876
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
use post when jql in params #548
Conversation
Please add a test for this change. |
original test already cover it |
@@ -2483,7 +2483,10 @@ def _get_url(self, path, base=JIRA_BASE_URL): | |||
|
|||
def _get_json(self, path, params=None, base=JIRA_BASE_URL): | |||
url = self._get_url(path, base) | |||
r = self._session.get(url, params=params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be conditionally switching from get to post here like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@264768502 : Is there any reason not to just POST all the time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get jql from filter first, them try to use jql to do issue search
JIRA server feedback input parameter with invalid string.
After switch to use post, search success.
But it has a little strange, after I switch another os, issue go. I have no idea why, maybe use different requests.
My commit maybe not best resolution.
As you know, http url has length limitation
and 'get' need encode parameter to url, this will involve some unknown issue.
how about add parameter to let user control use post or get?
In my opinion, check url will give a little hard to maintain
@asqui I don't want change original design too much
Only switch to post when search issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be conditionally switching from get to post as this code is doing. If we really need to do the switch inside _get_json
then please check the path being referenced in addition to the parameters.
9934c2d
to
524a37e
Compare
d097e2b
to
caf5944
Compare
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Sometimes, use a filter.jql to search issue, http get couldn't work well.
Switch to use post according formal recommended
it can also fix #336