Bug #16141
closed[controller] Missing collection's new fields on responses
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:/#
Updated by Lucas Di Pentima about 5 years ago
- Assigned To set to Lucas Di Pentima
- Status changed from New to In Progress
Updated by Lucas Di Pentima about 5 years ago
Fix at 7d1970e46 - branch 16141-gosdk-missing-fields
Test run: 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.
Updated by Tom Clegg about 5 years 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).
Updated by Lucas Di Pentima about 5 years ago
Updates at ebd4760f0
Test run: developer-run-tests: #1722
- Addresses above suggestions ignoring the
href
field, and fixing someCollection
type's field declarations. - Expands test case to check for other object types.
- Fixes
User
's fixture and GoSDK type.
Updated by Lucas Di Pentima about 5 years 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: developer-run-tests-remainder: #1789 /console
Updated by Lucas Di Pentima about 5 years ago
Fix on 484cf9fec
Test run: 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.
Updated by Tom Clegg about 5 years 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.
Updated by Lucas Di Pentima about 5 years ago
Updates at 64dc3f43d
Test run: developer-run-tests-remainder: #1792
- Fixed the Collection type.
- Updated the test to use a more complete collection fixture.
Updated by Lucas Di Pentima about 5 years 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: developer-run-tests: #1724
Updated by Lucas Di Pentima about 5 years ago
- Status changed from In Progress to Resolved