Bug #16141

[controller] Missing collection's new fields on responses

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

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
02/07/2020
Due date:
% Done:

100%

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

Description

There're some fields missing on the golang SDK, and the controller isn't including them on the responses.

OTOH, Workbench2 seems to get some of those fields when asking for group's contents.

Below is an example of asking the railsAPI vs controller on an arvbox instance:

root@6c2d65fcba97:/# curl -k -H "Authorization: OAuth2 <token>" http://localhost:8004/arvados/v1/collections/xz8sq-4zz18-00kn5pa4uizijr8 | jq .file_count
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 1932k    0 1932k    0     0  8541k      0 --:--:-- --:--:-- --:--:-- 8553k
39354
root@6c2d65fcba97:/# curl -k -H "Authorization: OAuth2 <token>" https://localhost:8000/arvados/v1/collections/xz8sq-4zz18-00kn5pa4uizijr8 | jq .file_count
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 1932k    0 1932k    0     0  4865k      0 --:--:-- --:--:-- --:--:-- 4868k
null
root@6c2d65fcba97:/# 


Subtasks

Task #16142: Review 16141-gosdk-missing-fieldsResolvedLucas Di Pentima

Associated revisions

Revision 6ef78156
Added by Lucas Di Pentima 5 months ago

Merge branch '16141-gosdk-missing-fields'

Closes #16141

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

History

#1 Updated by Lucas Di Pentima 5 months ago

  • Assigned To set to Lucas Di Pentima
  • Status changed from New to In Progress

#2 Updated by Lucas Di Pentima 5 months ago

Fix at 7d1970e46 - branch 16141-gosdk-missing-fields
Test run: https://ci.arvados.org/job/developer-run-tests/1720/

  • Adds missing collection fields from the RailsAPI on the GoSDK.
  • Adds test comparing direct RailsAPI response against controller's.

If this is the correct testing approach, we could expand it to the other object types.

#3 Updated by Tom Clegg 5 months ago

If this is the correct testing approach, we could expand it to the other object types.

Yes, this test (and expanding it to other types) looks like a good idea.

Why does the test avoid checking the controller response value when the Rails response value is nil? (Wouldn't the DeepEquals check still be fitting in this case?)

While we're here, does the converse test (new fields returned by controller that don't come from Rails) reveal anything interesting?

I don't think we should add "href". It seems to be broken in railsAPI anyway, currently reporting "/collections/$uuid".

version, file_count, and file_size_total are all "not null" in the database, so they should be int, not *int.

file_size_total should probably be int64, like file sizes elsewhere in Go (e.g., https://golang.org/pkg/os/#FileInfo).

#4 Updated by Lucas Di Pentima 5 months ago

Updates at ebd4760f0
Test run: https://ci.arvados.org/job/developer-run-tests/1722/

  • Addresses above suggestions ignoring the href field, and fixing some Collection type's field declarations.
  • Expands test case to check for other object types.
  • Fixes User's fixture and GoSDK type.

#5 Updated by Lucas Di Pentima 5 months ago

Added better failure message on 1da59913e

It seems that the keep_services' fixture is not loaded when running tests on Jenkins? The tests pass on my local dev env: https://ci.arvados.org/job/developer-run-tests-remainder/1789/console

#6 Updated by Lucas Di Pentima 5 months ago

Fix on 484cf9fec
Test run: https://ci.arvados.org/job/developer-run-tests-remainder/1790/

  • Instead of using a keep_service fixture UUID, request the list of keep services to the test server and use the first.

#7 Updated by Tom Clegg 5 months ago

Although description can be null in the database, null vs. "" doesn't seem like a meaningful distinction -- it can be a string rather than a *string.

Is there a reason why collection Version, user ModifiedByClientUUID, and user ModifiedByUserUUID need to be pointers? Database says "not null".

(Doesn't need to be changed, just fyi) you could have used nil instead of map[string]bool{} for all the empty maps in the test case, since nobody assigns to them.

#8 Updated by Lucas Di Pentima 5 months ago

Updates at 64dc3f43d
Test run: https://ci.arvados.org/job/developer-run-tests-remainder/1792/

  • Fixed the Collection type.
  • Updated the test to use a more complete collection fixture.

#9 Updated by Tom Clegg 5 months ago

LGTM, thanks!

#10 Updated by Lucas Di Pentima 5 months ago

Checking the merge commit previous to push, I realized I forgot to correct the User type, so I've just did that and also added the modified_by_*_uuid fields to the active user fixture, because the only one that had those fields was the System user, but it lacks of other fields.

Updates at 785bed2f7
Test run: https://ci.arvados.org/job/developer-run-tests/1724/

#11 Updated by Lucas Di Pentima 5 months ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF