Project

General

Profile

Actions

Bug #17319

closed

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

Added by Lucas Di Pentima about 3 years ago. Updated about 3 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench2
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/2021Actions
Actions #1

Updated by Peter Amstutz about 3 years ago

  • Assigned To set to Lucas Di Pentima
Actions #2

Updated by Lucas Di Pentima about 3 years ago

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

Updated by Lucas Di Pentima about 3 years ago

  • Status changed from New to In Progress
Actions #4

Updated by Lucas Di Pentima about 3 years 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 about 3 years 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 about 3 years ago

Updates at arvados-workbench2|6a66e260

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

Updated by Daniel Kutyła about 3 years 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 about 3 years 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 about 3 years 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 about 3 years ago

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

Also available in: Atom PDF