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

use post when jql in params #548

Closed
wants to merge 2 commits into from
Closed

Conversation

264768502
Copy link

@264768502 264768502 commented Apr 6, 2018

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

@dbaxa
Copy link
Member

dbaxa commented Apr 8, 2018

Please add a test for this change.

@264768502
Copy link
Author

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)
Copy link
Member

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.

Copy link
Contributor

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?

Copy link
Author

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.

Copy link
Member

@dbaxa dbaxa left a 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.

@hdost hdost changed the base branch from master to develop May 6, 2018 20:54
@hdost hdost force-pushed the develop branch 2 times, most recently from 9934c2d to 524a37e Compare June 29, 2018 12:44
@hdost hdost changed the base branch from develop to master July 2, 2018 21:20
@stale
Copy link

stale bot commented Oct 15, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in the JQL Query: The character '%' is a reserved JQL character.
5 participants