Skip to content

[BI-617] - Sort/paginate/filter for trait upload#49

Merged
ctucker3 merged 6 commits intodevelopfrom
feature/BI-617
Oct 15, 2020
Merged

[BI-617] - Sort/paginate/filter for trait upload#49
ctucker3 merged 6 commits intodevelopfrom
feature/BI-617

Conversation

@ctucker3
Copy link
Contributor

@ctucker3 ctucker3 commented Oct 6, 2020

Updated trait upload services to parse the JSONB data from the database before returning to controller. For the sorting/filtering/pagination we needed something we could read and act on, so that seemed like the most logically course. It doesn't change the way the data is returned.

There is one more point of failure in the upload process though. If for some reason the data was not stored properly during the file upload, there will be a failure when parsing this. But, that should never happen since the JSONB being stored in the database is rendered from Trait objects to begin with.

@Setter
@Accessors(chain=true)
@ToString
@SuperBuilder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guessing this was removed because there was an ambiguous "builder()" method? If so, the name of the builder for this class can be changed by updating the @Superbuilder annotation to something like: @SuperBuilder(builderMethodName = "uploadBuilder")

The use of the builder will need to be typed given that ProgramUpload now has generic support for the parsedData field. An example of how to do this would be: ProgramUpload.<T>uploadBuilder()...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect! Yeah the type ProgramUpload was messing things up and not liking ProgramUpload.<Trait>builder().... I'll try out the named builder and see how that goes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed and pushed

Comment on lines +60 to +69
public static <T> HttpResponse<Response<ProgramUpload>> getQueryResponse(
ProgramUpload upload, AbstractQueryMapper mapper, SearchRequest searchRequest, QueryParams queryParams) {
return processSearchResponse(upload, searchRequest, queryParams, mapper, new Metadata());
}

// Pagination and sort only
public static <T> HttpResponse<Response<ProgramUpload>> getQueryResponse(
ProgramUpload upload, AbstractQueryMapper mapper, QueryParams queryParams) {
return processSearchResponse(upload, null, queryParams, mapper, new Metadata());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be helpful to have the names of these methods reflect that they are for uploads instead of overloading the method name of getQueryResponse

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How so? I plan on changing the names, but still wondering your thoughts as to the benefits of changing the method name instead of overloading?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed and pushed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main reason for saying that is because of this HttpResponse<Response<ProgramUpload>> being specifically typed to ProgramUpload, and the overloaded method has it as generice <T>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created new methods for ProgramUpload and removed the generic typing from those methods.

return HttpResponse.ok(new Response(metadata, new DataResponse(paginationResult.getLeft())));
}

private static <T> HttpResponse<Response<ProgramUpload>> processSearchResponse(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing goes here for the method name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed and pushed

@ctucker3 ctucker3 merged commit c2cea39 into develop Oct 15, 2020
@ctucker3 ctucker3 deleted the feature/BI-617 branch October 15, 2020 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants