-
Notifications
You must be signed in to change notification settings - Fork 264
Support nextPageToken and the the new Jira /search/jql endpoint (#6015) #6063
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,16 +26,18 @@ | |
import static org.opensearch.dataprepper.logging.DataPrepperMarkers.NOISY; | ||
import static org.opensearch.dataprepper.plugins.source.jira.utils.JqlConstants.EXPAND_FIELD; | ||
import static org.opensearch.dataprepper.plugins.source.jira.utils.JqlConstants.EXPAND_VALUE; | ||
import static org.opensearch.dataprepper.plugins.source.jira.utils.JqlConstants.FIELDS_FIELD; | ||
import static org.opensearch.dataprepper.plugins.source.jira.utils.JqlConstants.FIELDS_VALUE; | ||
import static org.opensearch.dataprepper.plugins.source.jira.utils.JqlConstants.JQL_FIELD; | ||
|
||
@Slf4j | ||
@Named | ||
public class JiraRestClient extends AtlassianRestClient { | ||
|
||
public static final String REST_API_SEARCH = "rest/api/3/search"; | ||
public static final String REST_API_SEARCH = "rest/api/3/search/jql"; | ||
public static final String REST_API_FETCH_ISSUE = "rest/api/3/issue"; | ||
public static final String FIFTY = "50"; | ||
public static final String START_AT = "startAt"; | ||
public static final String NEXT_PAGE_TOKEN = "nextPageToken"; | ||
public static final String MAX_RESULT = "maxResults"; | ||
private static final String TICKET_FETCH_LATENCY_TIMER = "ticketFetchLatency"; | ||
private static final String SEARCH_CALL_LATENCY_TIMER = "searchCallLatency"; | ||
|
@@ -72,16 +74,30 @@ public JiraRestClient(RestTemplate restTemplate, AtlassianAuthConfig authConfig, | |
* @param startAt the start at | ||
* @return InputStream input stream | ||
*/ | ||
public SearchResults getAllIssues(StringBuilder jql, int startAt) { | ||
public SearchResults getAllIssues(StringBuilder jql, String nextPageToken) { | ||
|
||
String url = authConfig.getUrl() + REST_API_SEARCH; | ||
|
||
URI uri = UriComponentsBuilder.fromHttpUrl(url) | ||
.queryParam(MAX_RESULT, FIFTY) | ||
.queryParam(START_AT, startAt) | ||
.queryParam(JQL_FIELD, jql) | ||
.queryParam(EXPAND_FIELD, EXPAND_VALUE) | ||
.buildAndExpand().toUri(); | ||
URI uri; | ||
|
||
if(nextPageToken!= null && !nextPageToken.isBlank() && !nextPageToken.isEmpty()){ | ||
uri = UriComponentsBuilder.fromHttpUrl(url) | ||
.queryParam(MAX_RESULT, FIFTY) | ||
.queryParam(NEXT_PAGE_TOKEN, nextPageToken) | ||
.queryParam(JQL_FIELD, jql) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Subsequent calls that are just fetching the next page, doesn't require There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Valid |
||
.queryParam(EXPAND_FIELD, EXPAND_VALUE) | ||
.queryParam(FIELDS_FIELD, FIELDS_VALUE) | ||
.buildAndExpand().toUri(); | ||
} | ||
else{ | ||
uri = UriComponentsBuilder.fromHttpUrl(url) | ||
.queryParam(MAX_RESULT, FIFTY) | ||
.queryParam(JQL_FIELD, jql) | ||
.queryParam(EXPAND_FIELD, EXPAND_VALUE) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like, with the new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
.queryParam(FIELDS_FIELD, FIELDS_VALUE) | ||
.buildAndExpand().toUri(); | ||
} | ||
|
||
return searchCallLatencyTimer.record( | ||
() -> { | ||
try { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,4 +27,6 @@ public class JqlConstants { | |
public static final String JQL_FIELD = "jql"; | ||
public static final String EXPAND_FIELD = "expand"; | ||
public static final String EXPAND_VALUE = "all"; | ||
Comment on lines
28
to
29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two fields can be removed as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per above comment, JIRA docs have both expand and fields. Maybe expand will be useful in the future? |
||
public static final String FIELDS_FIELD = "fields"; | ||
public static final String FIELDS_VALUE = "*all"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} |
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.
could have been simpler to rely on
isLast
?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 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..