Project

General

Profile

Actions

Bug #17319

closed

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

Added by Lucas Di Pentima over 1 year ago. Updated over 1 year 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 1 (0 open1 closed)

Arvados - Task #17322: Review 17319-service-layer-uuid-validationClosedDaniel Kutyła02/19/2021

Actions
Actions #1

Updated by Peter Amstutz over 1 year ago

  • Assigned To set to Lucas Di Pentima
Actions #2

Updated by Lucas Di Pentima over 1 year ago

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

Updated by Lucas Di Pentima over 1 year ago

  • Status changed from New to In Progress
Actions #4

Updated by Lucas Di Pentima over 1 year ago

Fix at arvados-workbench2|0429d378 - branch 17319-service-layer-uuid-validation
Test run: developer-tests-workbench2: #293

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

Updated by Daniel Kutyła over 1 year 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

Actions #6

Updated by Lucas Di Pentima over 1 year ago

Updates at arvados-workbench2|6a66e260

  • Moves mock's init to beforeEach()
Actions #7

Updated by Daniel Kutyła over 1 year 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?

Actions #8

Updated by Lucas Di Pentima over 1 year 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!

Actions #9

Updated by Lucas Di Pentima over 1 year ago

Update at arvados-workbench2|eb40293b

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

Actions #10

Updated by Anonymous over 1 year ago

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

Also available in: Atom PDF