Support nextPageToken and the the new Jira /search/jql endpoint (#6015) #6063
Conversation
danielhoult
requested review from
sb2k16,
chenqi0805,
engechas,
san81,
srikanthjg,
graytaylor0,
dinujoh,
kkondaka,
KarstenSchnitter,
dlvenable and
oeyh
as code owners
| nextPageToken = searchIssues.getNextPageToken(); | ||
| addItemsToQueue(issueList, itemInfoQueue); | ||
| } while (startAt < total); | ||
| } while (nextPageToken != null && !nextPageToken.isEmpty() && !nextPageToken.isBlank()); |
There was a problem hiding this comment.
could have been simpler to rely on isLast?
There was a problem hiding this comment.
I thought about that, but there could be a case where isLast is false but maybe a bug from Atlassian (yes, bugs even impact large companies) where we don't receive a nextPageToken. Ultimately, regardless of the value of isLast, we need nextPageToken to be a non-empty value to get the next page. Defensive programming..
| uri = UriComponentsBuilder.fromHttpUrl(url) | ||
| .queryParam(MAX_RESULT, FIFTY) | ||
| .queryParam(NEXT_PAGE_TOKEN, nextPageToken) | ||
| .queryParam(JQL_FIELD, jql) |
There was a problem hiding this comment.
Subsequent calls that are just fetching the next page, doesn't require jql query param.
| uri = UriComponentsBuilder.fromHttpUrl(url) | ||
| .queryParam(MAX_RESULT, FIFTY) | ||
| .queryParam(JQL_FIELD, jql) | ||
| .queryParam(EXPAND_FIELD, EXPAND_VALUE) |
There was a problem hiding this comment.
It looks like, with the new FIELDS_FIELD query param added, we don't need previous EXPAND_FIELD. It can be removed in both the blocks
There was a problem hiding this comment.
The Jira API documentation still reference both 'expand' and 'fields' so I assumed there is a use-case for both. Without assuming functionality, I felt it better to leave both. https://developer.atlassian.com/cloud/jira/platform/rest/v3/api-group-issue-search/#api-rest-api-3-search-jql-post
There was a problem hiding this comment.
For this use case, we only need one of them. So we can just go with the newly introduced field describing the specific fields we want to fetch while searching through the tickets.
| public static final String EXPAND_FIELD = "expand"; | ||
| public static final String EXPAND_VALUE = "all"; |
There was a problem hiding this comment.
These two fields can be removed as well?
There was a problem hiding this comment.
Per above comment, JIRA docs have both expand and fields. Maybe expand will be useful in the future?
| public static final String EXPAND_FIELD = "expand"; | ||
| public static final String EXPAND_VALUE = "all"; | ||
| public static final String FIELDS_FIELD = "fields"; | ||
| public static final String FIELDS_VALUE = "*all"; |
There was a problem hiding this comment.
The query can be optimized to only fetch the specific fields needed for crawling while navigating over search results. While fetching the page, it can fetch all fields.
There was a problem hiding this comment.
I would like to make sure that I understand. Are you saying that an initial query is made to retrieve a list of issues, in which case we only need the id/key. Then those are added to a queue, and an iterator fetches the issues one by one?
I suspected that this may be the case, but was not sure.
There was a problem hiding this comment.
Yes, that is right. Initial search query returns the he list of issues with id, project, created datetime and last modified datetime attributes and then an other threads kicks in to fetch those items individually.
|
@danielhoult Could you please take care of these minor comments so that we can go ahead and merge these changes? |
Description
Support nextPageToken and the the new Jira /search/jql endpoint
Issues Resolved
Resolves #6015
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.