-
Notifications
You must be signed in to change notification settings - Fork 29
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
Fix IncrementAnIndexPaginationIterator #61
base: develop
Are you sure you want to change the base?
Fix IncrementAnIndexPaginationIterator #61
Conversation
@VictorD-Veolia: possible to share the exact problem that this fixes? Some sample response that this fails for? Also, could you add a test? |
The "Increment An Index" pagination is not usable at all. Having a pipeline with an HTTP plugin using "Increment an Index" pagination will throw "Failed to configure pipeline: Stage 'HTTP' encountered : Null error occurred while configuring the stage HTTP." at pipeline startup (with every configuration and independently of input data). |
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.
- Please create a Jira that describes the issue and link to it on this PR
- Please add a unit test
@@ -31,9 +31,6 @@ | |||
import java.util.List; | |||
import java.util.Map; | |||
|
|||
/** | |||
* Returns elements from json one by one by given json path. |
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.
why was this removed?
this.json = JSONUtil.toJsonElement(httpResponse.getBody()); | ||
this.schema = config.getSchema(); | ||
this.optionalFields = getOptionalFields(); | ||
|
||
JsonElement jsonElement = json; | ||
if (json.isJsonObject()) { | ||
JSONUtil.JsonQueryResponse queryResponse = | ||
JSONUtil.getJsonElementByPath(json.getAsJsonObject(), config.getResultPath(), optionalFields); | ||
JSONUtil.getJsonElementByPath(json.getAsJsonObject(), config.getResultPath(), optionalFields); |
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.
revert indent change
this.iterator = Collections.singleton(jsonElement).iterator(); | ||
} else { | ||
throw new IllegalArgumentException(String.format("Element found by '%s' json path is expected to be an object " + | ||
"or an array. Primitive found", config.getResultPath())); | ||
"or an array. Primitive found", config.getResultPath())); |
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.
revert indent change
* Example next element: | ||
* { |
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.
why was this changed?
* { | ||
* "key":"NETTY-13", |
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.
why was this changed?
@@ -196,7 +206,7 @@ private List<String> getOptionalFields() { | |||
public String getPrimitiveByPath(String path) { | |||
if (json.isJsonObject()) { | |||
JSONUtil.JsonQueryResponse queryResponse = JSONUtil.getJsonElementByPath(json.getAsJsonObject(), | |||
path, optionalFields); | |||
path, optionalFields); |
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.
revert indent change
JsonPage(BaseHttpSourceConfig config, HttpResponse httpResponse) { | ||
super(httpResponse); | ||
this.config = config; | ||
String body = httpResponse.getBody(); |
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.
where is this used?
@@ -43,17 +40,19 @@ class JsonPage extends BasePage { | |||
private final BaseHttpSourceConfig config; | |||
private final List<String> optionalFields; | |||
|
|||
// TODO : handle case where the result json object is null |
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.
what happens when result json object is null? are you planning to handle that case in another PR?
InvalidEntry<StructuredRecord> error = | ||
new InvalidEntry<>(1, "Couldn't find all required fields in the record", record); | ||
return new PageEntry(error, config.getErrorHandling()); | ||
if (iterator.hasNext()) { |
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.
you can avoid one level of indent if you return when there are no entries:
if (!iterartor.hasNext()) {
return new PageEntry(null);
}
...
Fix "Increment An Index" Pagination to match the expected behavior when "maxIndex" is null ("If empty, pagination will happen until the page with no elements.").