Add pagination to bugsnag and generic api controller #72

landonreed · 2020-09-10T17:09:28Z

Checklist

Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
The description lists all applicable issues this PR seeks to resolve
The description lists any configuration setting(s) that differ from the default settings
All tests and CI builds passing

Description

Adds pagination to bugsnag (our dev instance currently returns thousands of records) and the generic API controller. otp-admin-ui PR is ibi-group/otp-admin-ui#23 (still need changes in otp-react-redux)

…oller

codecov-commenter · 2020-09-22T20:50:47Z

Codecov Report

Merging #72 into dev will increase coverage by 0.27%.
The diff coverage is 65.26%.

@@             Coverage Diff              @@
##                dev      #72      +/-   ##
============================================
+ Coverage     47.74%   48.02%   +0.27%     
- Complexity      325      329       +4     
============================================
  Files            81       82       +1     
  Lines          2260     2299      +39     
  Branches        256      256              
============================================
+ Hits           1079     1104      +25     
- Misses         1054     1070      +16     
+ Partials        127      125       -2

Impacted Files	Coverage Δ	Complexity Δ
.../middleware/controllers/api/BugsnagController.java	`57.14% <0.00%> (-2.86%)`	`3.00 <0.00> (ø)`
...entripplanner/middleware/models/MonitoredTrip.java	`65.07% <ø> (ø)`	`21.00 <0.00> (ø)`
...opentripplanner/middleware/models/TripRequest.java	`81.25% <ø> (-1.11%)`	`5.00 <0.00> (-1.00)`
...iddleware/tripMonitor/jobs/MonitorAllTripsJob.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...anner/middleware/persistence/TypedPersistence.java	`52.00% <33.33%> (+2.72%)`	`18.00 <3.00> (+3.00)`
...rg/opentripplanner/middleware/utils/HttpUtils.java	`56.36% <50.00%> (-0.78%)`	`13.00 <3.00> (+2.00)`	⬇️
.../middleware/controllers/response/ResponseList.java	`75.00% <75.00%> (ø)`	`3.00 <3.00> (?)`
...nner/middleware/controllers/api/ApiController.java	`55.22% <80.76%> (+5.64%)`	`8.00 <0.00> (ø)`
...dleware/controllers/api/TripHistoryController.java	`72.72% <94.44%> (+6.06%)`	`4.00 <1.00> (-1.00)`	⬆️
.../opentripplanner/middleware/OtpMiddlewareMain.java	`55.10% <100.00%> (ø)`	`7.00 <1.00> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b9d54a...530f20e. Read the comment docs.

binh-dam-ibigroup

I will need to test some more. For now, the obvious changes are documenting the request parameters, and there is probably a way to request a subset of the data directly from Mongo and/or Bugsnag.

src/main/java/org/opentripplanner/middleware/controllers/api/ApiController.java

src/main/java/org/opentripplanner/middleware/controllers/api/BugsnagController.java

src/main/java/org/opentripplanner/middleware/controllers/response/ResponseList.java

br648

Looks good so approving.

evansiroky · 2020-09-29T18:06:45Z

src/main/java/org/opentripplanner/middleware/OtpMiddlewareMain.java

-            String route = HttpUtils.getRequiredQueryParamFromRequest(request, "route", false);
+            String route = getQueryParamFromRequest(request, "route", false);


I think using static imports like this might actually be a bad practice. See this perspective here.

What is your recommendation? That we qualify the method call with the util class? I'm fine with that, but we should define an approach for our projects generally on this. Perhaps we should create an issue and handle that in a separate project-wide PR?

Let's discuss with the team together sometime and then if we decide we want to do this, then we can handle all the changes in another PR.

src/main/java/org/opentripplanner/middleware/utils/HttpUtils.java

src/main/java/org/opentripplanner/middleware/controllers/response/ResponseList.java

evansiroky · 2020-09-29T18:59:05Z

src/main/resources/latest-spark-swagger-output.yaml

      responses:
        "200":
          description: "successful operation"
          schema:
-            $ref: "#/definitions/AdminUser[]"
+            $ref: "#/definitions/ResponseList"


I suppose this is a little better than the AdminUser[] type since the definition for that was just an empty object. It might be out of scope for this PR, but it would be nice to have the swagger docs include refs to the proper object in an array.

@evansiroky Note that AdminUser is a subclass of AbstractUser, but spark-swagger ignores inherited fields at the time this was implemented. We insert inherited fields into the AdminUserswagger type using the PublicApiDocGenerator class.

evansiroky · 2020-09-29T19:02:58Z

src/main/resources/latest-spark-swagger-output.yaml

+      data:
+        type: "array"
+        items:
+          type: "string"


This is going to be incorrect since at the very least the items will be an object. It might be necessary to have separate ResponseList types that include the correct reference to the subclass that they return.

@binh-dam-ibigroup, do you have any thoughts on how to ammend this swagger def?

The most thorough way to handle this would be to change the document generator class to create a ResponseList_Subtype object definition for each of the ResponseList responses we are publishing. We can also try creating a separate descendant of ResponseBody per @evansiroky's suggestion above and see how spark-swagger handles that.

To follow up on my comment above, you could modify class PublicApiDocsGenerator to do the following:

Scan all ResponseList return entries in the autogenerated swagger.

For each instance, find the type of the elements in the list from one of the parent elements.

Generate a custom return type definition that correspond to ResponseList<type>

Replace ResponseList with the custom type generated above.

Further discussion should be had in #85

evansiroky

See comments

…oc, static imports)

binh-dam-ibigroup

Some light documentation updates, then it should be good to go.

src/main/java/org/opentripplanner/middleware/controllers/api/BugsnagController.java

src/main/java/org/opentripplanner/middleware/controllers/api/ApiController.java

src/main/java/org/opentripplanner/middleware/controllers/api/TripHistoryController.java

src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java

src/main/java/org/opentripplanner/middleware/persistence/TypedPersistence.java

binh-dam-ibigroup · 2020-10-01T14:46:05Z

src/main/resources/latest-spark-swagger-output.yaml

      responses:
        "200":
          description: "successful operation"
          schema:
-            $ref: "#/definitions/AdminUser[]"
+            $ref: "#/definitions/ResponseList"


@evansiroky Note that AdminUser is a subclass of AbstractUser, but spark-swagger ignores inherited fields at the time this was implemented. We insert inherited fields into the AdminUserswagger type using the PublicApiDocGenerator class.

src/main/java/org/opentripplanner/middleware/controllers/api/TripHistoryController.java

src/test/java/org/opentripplanner/middleware/ApiUserFlowTest.java

…lazz

evansiroky

Build is failing
PR has merge conflicts
Figuring out documentation of ResponseList typing seems to be unresolved

binh-dam-ibigroup · 2020-10-07T14:53:46Z

Does a dependency need to be updated for the build to pass?

This reverts commit 0c800e4.

codecov-io · 2020-10-12T14:28:25Z

Codecov Report

Merging #72 into dev will increase coverage by 0.36%.
The diff coverage is 67.61%.

@@             Coverage Diff              @@
##                dev      #72      +/-   ##
============================================
+ Coverage     47.74%   48.11%   +0.36%     
- Complexity      325      329       +4     
============================================
  Files            81       82       +1     
  Lines          2260     2305      +45     
  Branches        256      256              
============================================
+ Hits           1079     1109      +30     
- Misses         1054     1071      +17     
+ Partials        127      125       -2

Impacted Files	Coverage Δ	Complexity Δ
...entripplanner/middleware/models/MonitoredTrip.java	`65.07% <ø> (ø)`	`21.00 <0.00> (ø)`
...opentripplanner/middleware/models/TripRequest.java	`81.25% <ø> (-1.11%)`	`5.00 <0.00> (-1.00)`
...iddleware/tripMonitor/jobs/MonitorAllTripsJob.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...anner/middleware/persistence/TypedPersistence.java	`52.00% <33.33%> (+2.72%)`	`18.00 <3.00> (+3.00)`
.../middleware/controllers/api/BugsnagController.java	`62.50% <35.71%> (+2.50%)`	`3.00 <0.00> (ø)`
...rg/opentripplanner/middleware/utils/HttpUtils.java	`56.36% <50.00%> (-0.78%)`	`13.00 <3.00> (+2.00)`	⬇️
.../middleware/controllers/response/ResponseList.java	`73.68% <73.68%> (ø)`	`3.00 <3.00> (?)`
...nner/middleware/controllers/api/ApiController.java	`55.22% <81.48%> (+5.64%)`	`8.00 <0.00> (ø)`
...dleware/controllers/api/TripHistoryController.java	`72.72% <94.73%> (+6.06%)`	`4.00 <1.00> (-1.00)`	⬆️
.../opentripplanner/middleware/OtpMiddlewareMain.java	`55.10% <100.00%> (ø)`	`7.00 <1.00> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b9d54a...7c1e133. Read the comment docs.

refactor(pagination): add pagination to bugsnag and generic api contr…

68168ba

…oller

landonreed mentioned this pull request Sep 10, 2020

Add pagination to dashboard list views ibi-group/otp-admin-ui#23

Merged

docs(swagger): update API docs

bdfc6a7

landonreed marked this pull request as ready for review September 22, 2020 20:57

landonreed requested review from binh-dam-ibigroup, br648 and evansiroky September 22, 2020 20:58

binh-dam-ibigroup requested changes Sep 24, 2020

View reviewed changes

binh-dam-ibigroup assigned landonreed Sep 24, 2020

refactor(pagination): refactor ResponseList to simplify construction

f6c361b

landonreed assigned evansiroky, br648 and binh-dam-ibigroup and unassigned landonreed Sep 28, 2020

landonreed requested a review from binh-dam-ibigroup September 28, 2020 16:10

landonreed added 11 commits September 28, 2020 12:14

docs(swagger): update swagger API docs

767ee7e

refactor(bugsnag): clean up code to construct event summaries

00dc9cd

Merge branch 'dev' into add-pagination

efab948

refactor(redirect): update HttpUtils refactored method name

5c1bfcf

test: fix broken e2e test

7795bd1

refactor: add no-arg constructor for ResponseList

02ea4b8

refactor: fix test and remove unused imports

7288d2b

refactor(api-key): replace customerId with tag for userId

a2146c3

refactor(pagination): change page param to offset

755a900

refactor(pagination): change remaining page -> offset instances

99d364b

docs(pagination): update swagger for pagination

af9ad88

br648 approved these changes Sep 29, 2020

View reviewed changes

br648 removed their assignment Sep 29, 2020

evansiroky reviewed Sep 29, 2020

View reviewed changes

src/main/java/org/opentripplanner/middleware/utils/HttpUtils.java Outdated Show resolved Hide resolved

evansiroky reviewed Sep 29, 2020

View reviewed changes

src/main/java/org/opentripplanner/middleware/utils/HttpUtils.java Outdated Show resolved Hide resolved

evansiroky reviewed Sep 29, 2020

View reviewed changes

src/main/java/org/opentripplanner/middleware/controllers/response/ResponseList.java Outdated Show resolved Hide resolved

evansiroky reviewed Sep 29, 2020

View reviewed changes

evansiroky suggested changes Sep 29, 2020

View reviewed changes

evansiroky assigned landonreed and unassigned evansiroky Sep 29, 2020

landonreed mentioned this pull request Sep 29, 2020

Update API calls to use pagination endpoint opentripplanner/otp-react-redux#238

Merged

landonreed added 2 commits September 30, 2020 10:20

refactor(HttpUtils): refactor getQueryParamFromRequest (params, javad…

affc874

…oc, static imports)

refactor(HttpUtils): fix max int value check

530f20e

binh-dam-ibigroup requested changes Oct 1, 2020

View reviewed changes

binh-dam-ibigroup removed their assignment Oct 1, 2020

landonreed assigned evansiroky Oct 2, 2020

landonreed added 2 commits October 2, 2020 14:26

refactor(pagination): update swagger descriptions; add ResponseList#c…

7e59e7a

…lazz

build(deps): use ibi-group fork of spark-swagger

0c800e4

evansiroky suggested changes Oct 5, 2020

View reviewed changes

evansiroky removed their assignment Oct 5, 2020

landonreed mentioned this pull request Oct 9, 2020

Issue with generic types in API docs generation #85

Open

landonreed added 2 commits October 12, 2020 10:24

Revert "build(deps): use ibi-group fork of spark-swagger"

44c6c77

This reverts commit 0c800e4.

docs(swagger): update API docs

7c1e133

landonreed added 3 commits October 12, 2020 10:31

Merge branch 'dev' into add-pagination

d6e236d

test: fix trip history persistence compilation error

ace5772

refactor(ApiGatewayUtils): use fluent method for inlining timeout call

692c66a

landonreed merged commit 75b1e06 into dev Oct 12, 2020

landonreed deleted the add-pagination branch October 12, 2020 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pagination to bugsnag and generic api controller #72

Add pagination to bugsnag and generic api controller #72

landonreed commented Sep 10, 2020 •

edited

Loading

codecov-commenter commented Sep 22, 2020 •

edited

Loading

binh-dam-ibigroup left a comment

br648 left a comment

evansiroky Sep 29, 2020

landonreed Sep 30, 2020

evansiroky Oct 5, 2020

evansiroky Sep 29, 2020

binh-dam-ibigroup Oct 1, 2020

evansiroky Sep 29, 2020

landonreed Sep 30, 2020

binh-dam-ibigroup Sep 30, 2020

binh-dam-ibigroup Sep 30, 2020

landonreed Oct 9, 2020

evansiroky left a comment

binh-dam-ibigroup left a comment

binh-dam-ibigroup Oct 1, 2020

evansiroky left a comment

binh-dam-ibigroup commented Oct 7, 2020

codecov-io commented Oct 12, 2020

		String route = HttpUtils.getRequiredQueryParamFromRequest(request, "route", false);
		String route = getQueryParamFromRequest(request, "route", false);

Add pagination to bugsnag and generic api controller #72

Add pagination to bugsnag and generic api controller #72

Conversation

landonreed commented Sep 10, 2020 • edited Loading

Checklist

Description

codecov-commenter commented Sep 22, 2020 • edited Loading

Codecov Report

binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

br648 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evansiroky left a comment

Choose a reason for hiding this comment

binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evansiroky left a comment

Choose a reason for hiding this comment

binh-dam-ibigroup commented Oct 7, 2020

codecov-io commented Oct 12, 2020

Codecov Report

landonreed commented Sep 10, 2020 •

edited

Loading

codecov-commenter commented Sep 22, 2020 •

edited

Loading