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.

Sign up for GitHub

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

Jump to bottom

Convert old rest stack: supremm module #28

Merged

jpwhite4 merged 1 commit into ubccr:xdmod6.6 from jsperhac:convert-old-rest-stack

Apr 11, 2017

Contributor

jsperhac commented Mar 23, 2017 •

edited

Loading

This is a step along the way toward killing off the old REST stack.

Description

Create a NewRest-style controller for the internal dashboard's supremm dataflow functionality, to replace the old classes/REST/supremm code.

Refer to pull requests against

xdmod (Convert old rest stack: base xdmod xdmod#73), that: removes entirely-unused source code from the old classes/REST rest stack, and removes associated documentation, html/rest_catalog/
xdmod-appkenel (Convert old rest stack: appkernels module xdmod-appkernels#12), that converts another old REST stack remnant to use the NewRest controllers.
old pull request #422 from the xdmod-private ccr repo, which did not go quite this far in clearing unused code from the old REST stack

Motivation and Context

Head in the direction of reducing 3 rest stacks to 2 rest stacks
I'm tired of duplicate code.

Tests performed

Changes are limited to the internal dashboard's supremm dataflow, which reports on the last time various supremm sources contributed data. These were tested by hand to verify, using chrome debugger to observe which rest endpoints are in use. Finally, tests for the new endpoints have been added and use the integration test harness.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project as found in the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

Caveats:

More things I'd like to do related to the rest stacks (but not a part of the current pull request):

Introduce improved logging for the NewRest controllers

To Do:

Remaining to do in this exercise:

see pull requests named "convert-old-rest-stack" for appkernel and base xdmod modules.
following merging of the convert-old-rest-stack pull requests, delete entire contents of REST/, and review the contents of libraries/rest.php. This will be addressed in a subsequent pull request.
Move this and other iframes in the internal dashboard into proper tabs. (And: Review ~duplicated code such as internal_dashboard/user_check.php and internal_dashboard/supremm/user_check.php.) This should be addressed in a separate pull request.

This was referenced Mar 23, 2017

Convert old rest stack: base xdmod ubccr/xdmod#73

Merged

Convert old rest stack: appkernels module ubccr/xdmod-appkernels#12

Merged

jpwhite4 requested changes

View reviewed changes

classes/NewRest/Controllers/SupremmDataflowControllerProvider.php Outdated

+                          } else {
+                              $payload['message'] = self::_DEFAULT_ERROR_MESSAGE;
+                          }
+                      } catch (BadRequestHttpException $e) {

Member

jpwhite4 Mar 23, 2017

Is it necessary to catch all of these exceptions? Surely the middleware should handle this for you automatically.

Contributor Author

jsperhac Mar 23, 2017

This is modeled rather heavily on the other NewRest controllers, which similarly catch lots of exceptions. I will review...

Contributor Author

jsperhac Mar 29, 2017

Exception code has been removed; framework handles this, as you point out!

classes/NewRest/Controllers/SupremmDataflowControllerProvider.php Outdated

+                          ->get("$root/dbstats", "$base::getDbstats")
+                          ->assert('resource_id', '(\w|_|-])+')
+                          ->convert('db_id', "$conversions::toInt")
+                          ->convert('resource_id', "$conversions::toString");

Member

jpwhite4 Mar 23, 2017

This definition says that db_id is an int and resource_id is a string, but on lines 123 and 124 you require the exact opposite: get db_id must be a string and resource_id must be an int.

Contributor Author

jsperhac Mar 23, 2017

Yup, it's wrong here, I will fix. Maybe changing the variable name will also help clarify this.

classes/NewRest/Controllers/SupremmDataflowControllerProvider.php Outdated

+                      } catch (HttpException $e) {
+                          $payload['message'] = $e->getMessage();
+                          $statusCode = $e->getStatusCode();
+                      } catch (\Exception $e) {

Member

jpwhite4 Mar 23, 2017

If authorize() throws an AccessDeniedException then this code will catch it and change the status code to internal error. This is wrong.

I think that you should not be catching exceptions here. Instead let the framework handle them

plessbd reviewed

View reviewed changes

classes/NewRest/Controllers/SupremmDataflowControllerProvider.php Outdated

+                   * @param timestamp $ts
+                   * @return mixed
+                   */
+                  private function _time2str($ts)

Contributor

plessbd Mar 23, 2017

I feel like this should be a utility function that anything can use. Maybe even remove this from the backend code and use something like moment in the frontend to do this as a display thing...

Contributor Author

jsperhac Mar 28, 2017

For the moment the idea is to port what's there. Moving general functionality into utility functions should absolutely be done separately...

plessbd previously requested changes

View reviewed changes

classes/NewRest/Controllers/SupremmDataflowControllerProvider.php Outdated

+                   * @param numeric $d
+                   * @return mixed
+                   */
+                  private function _formatDataSize($d)

Contributor

plessbd Mar 23, 2017

This as well seems like a utility function that is able to be used in multiple places.

Contributor Author

jsperhac Mar 28, 2017

As mentioned above, yes, I agree--in a separate pull request.

Contributor

plessbd commented Mar 23, 2017

I think it would help to run the code through phpcbf to fix a bunch of the warnings and errors from travis ci.

Contributor Author

jsperhac commented Mar 23, 2017

And I missed that travis ci failed. OK, I'll revisit. Thanks.

jpwhite4 requested changes

View reviewed changes

html/internal_dashboard/supremm/stats.php Outdated

+                      <script type="text/javascript" src="/gui/lib/extjs/ext-all-debug.js"></script>
+                      <!-- RESTProxy -->
+                      <script type="text/javascript" src="../../gui/js/CCR.js"></script>

Member

jpwhite4 Mar 28, 2017

Why are you including the REST.js RESTProxy and CCR.js javascript files? They are not needed.

Contributor Author

jsperhac Mar 28, 2017

I believe you are right...duly noted, I'll remove those includes.

Contributor Author

jsperhac Mar 29, 2017

Done.

jsperhac force-pushed the convert-old-rest-stack branch from b6c9614 to 7a05970 Compare

March 29, 2017 20:10

jsperhac dismissed plessbd’s stale review

March 30, 2017 14:50

Moving utility/format functions will be done in a separate pull request.

jsperhac mentioned this pull request

Fix exception throw in XdmodTestHelper ubccr/xdmod#81

Merged

6 tasks

Contributor Author

jsperhac commented Mar 31, 2017

In order for integration tests to work, the fix-framework-authorize-exceptions branch on xdmod repo will be needed (pull request to follow once unit tests exist)

jsperhac mentioned this pull request

Fixed exception instantiations in BaseControllerProvider ubccr/xdmod#87

Merged

6 tasks

jsperhac force-pushed the convert-old-rest-stack branch 4 times, most recently from 9dcf0e8 to 1bbe21d Compare

April 11, 2017 17:54

tyearke mentioned this pull request

Travis: Cache npm cache directory instead of node_modules directories #34

Merged

2 tasks

plessbd approved these changes

View reviewed changes

Contributor

plessbd commented Apr 11, 2017

Existing travis CI Errors will be fixed by #35 and an update to https://github.com/jpwhite4/lint-diff

plessbd mentioned this pull request

Fix pull #28 #33

Closed

6 tasks


          Convert internal dashboard supremm dataflow from old REST stack to Ne…

d5d81d8

…wRest stack,

to enable elimination of old REST stack code.

jsperhac force-pushed the convert-old-rest-stack branch from 0c6114a to d5d81d8 Compare

April 11, 2017 18:44

jpwhite4 approved these changes

View reviewed changes

jpwhite4 merged commit d7558a2 into ubccr:xdmod6.6

tyearke added this to the v6.6.0 milestone

tyearke added the qa label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

qa