Bug #17319

CommonService.get(uuid) should throw an exception when uuid is empty string

Added by Lucas Di Pentima 3 months ago. Updated 3 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench2
Start date:
02/19/2021
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

Currently if uuid is "", the service layer does a request to the backend with an incorrect URL, for example: arvados/v1/container_requests/, ultimately getting a 404 response.

RailsAPI may accept trailing slashes on some endpoints, but as we're migrating those endpoint handlings to controller, these kind of issues may start to creep up of different parts of the app.


Subtasks

Arvados - Task #17322: Review 17319-service-layer-uuid-validationClosedDaniel Kutyła

Associated revisions

Revision 565a4007 (diff)
Added by Lucas Di Pentima 3 months ago

Avoids 404 responses when no container request is selected.
Refs #17319

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

Revision 499628c0 (diff)
Added by Lucas Di Pentima 3 months ago

Avoids 404 responses when no container request is selected.
Refs #17319

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

Revision 1492885f
Added by Lucas Di Pentima 3 months ago

Merge branch '17319-service-layer-uuid-validation'
Closes #17319

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

History

#1 Updated by Peter Amstutz 3 months ago

  • Assigned To set to Lucas Di Pentima

#2 Updated by Lucas Di Pentima 3 months ago

  • Target version changed from 2021-02-17 sprint to 2021-03-03 sprint

#3 Updated by Lucas Di Pentima 3 months ago

  • Status changed from New to In Progress

#4 Updated by Lucas Di Pentima 3 months ago

Fix at arvados-workbench2|0429d378 - branch 17319-service-layer-uuid-validation
Test run: https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/293/

  • Validates that uuid is not an empty string on CommonService's get(), update() and delete() calls.
  • Adds unit tests.

#5 Updated by Daniel Kutyła 3 months ago

src\services\common-service\common-service.test.ts

const axiosInstance = axios.create();
// const axiosMock = new MockAdapter(axiosInstance);
const commonService = new CommonService(axiosInstance, "resource", actions);

please use mock and wrap this code in the beforeEach

#6 Updated by Lucas Di Pentima 3 months ago

Updates at arvados-workbench2|6a66e260

  • Moves mock's init to beforeEach()

#7 Updated by Daniel Kutyła 3 months ago

        commonService = new CommonService<any>(axiosInstance, "resource", actions);
        axiosInstance = axios.create();

should be

        axiosInstance = axios.create();
        commonService = new CommonService<any>(axiosInstance, "resource", actions);

as on a first call axiosInstance is null, on the other hand are you sure that you want to use axios without mocking within the test file?

#8 Updated by Lucas Di Pentima 3 months ago

Updates at arvados-workbench2|0c04dddf

I've cleaned up the test code a little bit, the axios instance is created just to satisfy the CommonService constructor, it's not being used on the test cases.
AFAICT, the axios mock would allow me to re-record test responses but in these test cases I'm not testing that, I'm just confirming that exceptions are being thrown when uuid=="" on some service calls.

Could you explain a little bit about your concerns? I'm not too experienced on TypeScript/Javascript testing. Thanks!

#9 Updated by Lucas Di Pentima 3 months ago

Update at arvados-workbench2|eb40293b

Don't use a real AxiosInstance when not needed, to avoid potential issue with future test authors.

#10 Updated by Anonymous 3 months ago

  • % Done changed from 0 to 100
  • Status changed from In Progress to Resolved

Also available in: Atom PDF