search

fablabbcn / smartcitizen-api

The Smart Citizen Engine
https://developer.smartcitizen.me
GNU Affero General Public License v3.0
10 stars 4 forks source link

Add tests for advertised ransack parameters and fix regressions #257

Closed timcowlishaw closed 11 months ago

timcowlishaw commented 1 year ago

Fixes #256 : Our documentation advertises that users, devices, and sensors are searchable with ransack - this commit adds some tests that assert (in a very basic manner) that this will succeed, and fixes a few cases where we weren't actually supporting the behaviour we advertise.

oscgonfer commented 1 year ago

Hi @timcowlishaw ,

Maybe I need some time to check the tests with you, I am unclear about some outputs of the test.

In particular, the ransack test gives a warning, I am sure you have already noticed it, but just checking if we can avoid a deprecation warning for the future:

https://github.com/fablabbcn/smartcitizen-api/actions/runs/6309556111/job/17129701742?pr=257#step:7:352

oscgonfer commented 1 year ago

Hi @timcowlishaw

Not related to the changes of this PR, but something to discuss (although not sure if it's something we can solve):

After testing queries on staging, I see many queries to the Postprocessing table relationships that slows down the search.

This ends up on things like this (notice the large query at the beginning and all the other Postprocessing queries:

Rendering v0/devices/index.jbuilder
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.030922 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Device Load (37.8ms)  SELECT DISTINCT "devices".* FROM "devices" WHERE (devices.workflow_state = 'active') AND "devices"."is_private" = $1 LIMIT $2 OFFSET $3  [["is_private", false], ["LIMIT", 25], ["OFFSET", 7600]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.034939 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   User Load (0.8ms)  SELECT "users".* FROM "users" WHERE "users"."workflow_state" = $1 AND "users"."id" IN ($2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16)  [["workflow_state", "active"], ["id", 8634], ["id", 8543], ["id", 8635], ["id", 8638], ["id", 8401], ["id", 8639], ["id", 8631], ["id", 8503], ["id", 8357], ["id", 8644], ["id", 8645], ["id", 5854], ["id", 8646], ["id", 8483], ["id", 6556]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.038076 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   DevicesTag Load (0.4ms)  SELECT "devices_tags".* FROM "devices_tags" WHERE "devices_tags"."device_id" IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, $21, $22, $23, $24, $25)  [["device_id", 16193], ["device_id", 16194], ["device_id", 16195], ["device_id", 16197], ["device_id", 16198], ["device_id", 16199], ["device_id", 16200], ["device_id", 16201], ["device_id", 16202], ["device_id", 16203], ["device_id", 16204], ["device_id", 16207], ["device_id", 16208], ["device_id", 16211], ["device_id", 16212], ["device_id", 16213], ["device_id", 16214], ["device_id", 16215], ["device_id", 16216], ["device_id", 16219], ["device_id", 16221], ["device_id", 16223], ["device_id", 16224], ["device_id", 16225], ["device_id", 16226]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.040431 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Tag Load (0.3ms)  SELECT "tags".* FROM "tags" WHERE "tags"."id" IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13)  [["id", 8], ["id", 17], ["id", 67], ["id", 71], ["id", 118], ["id", 114], ["id", 121], ["id", 119], ["id", 122], ["id", 27], ["id", 25], ["id", 34], ["id", 7]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.062556 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Component Load (20.6ms)  SELECT "components".* FROM "components" WHERE "components"."device_id" IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, $21, $22, $23, $24, $25)  [["device_id", 16193], ["device_id", 16194], ["device_id", 16195], ["device_id", 16197], ["device_id", 16198], ["device_id", 16199], ["device_id", 16200], ["device_id", 16201], ["device_id", 16202], ["device_id", 16203], ["device_id", 16204], ["device_id", 16207], ["device_id", 16208], ["device_id", 16211], ["device_id", 16212], ["device_id", 16213], ["device_id", 16214], ["device_id", 16215], ["device_id", 16216], ["device_id", 16219], ["device_id", 16221], ["device_id", 16223], ["device_id", 16224], ["device_id", 16225], ["device_id", 16226]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.072387 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Sensor Load (0.6ms)  SELECT "sensors".* FROM "sensors" WHERE "sensors"."id" IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, $21, $22, $23, $24, $25, $26, $27, $28, $29, $30, $31, $32, $33, $34, $35)  [["id", 14], ["id", 53], ["id", 55], ["id", 56], ["id", 58], ["id", 113], ["id", 10], ["id", 112], ["id", 88], ["id", 87], ["id", 89], ["id", 172], ["id", 174], ["id", 175], ["id", 177], ["id", 179], ["id", 180], ["id", 125], ["id", 126], ["id", 127], ["id", 128], ["id", 129], ["id", 130], ["id", 131], ["id", 4], ["id", 5], ["id", 160], ["id", 161], ["id", 158], ["id", 166], ["id", 167], ["id", 168], ["id", 169], ["id", 170], ["id", 165]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.074895 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16193], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.077821 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16194], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.080512 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16195], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.082605 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16197], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.084800 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16198], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.088284 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.3ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16199], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.090503 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.4ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16200], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.091901 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16201], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.093606 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16202], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.095503 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16203], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.097012 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16204], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.099423 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.3ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16207], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.101051 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16208], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.102848 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16211], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.104552 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16212], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.106296 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16213], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.108269 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16214], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.109982 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16215], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.111688 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16216], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.113857 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16219], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.115516 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16221], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.117623 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16223], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.119335 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16224], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.121110 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.2ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16225], ["LIMIT", 1]]
smartcitizen-api-app-1  | D, [2023-10-02T09:01:52.123018 #1] DEBUG -- : [91.126.229.112] [8ddfc7c6-4698-4d8b-a911-b912fe4ca27c]   Postprocessing Load (0.3ms)  SELECT "postprocessings".* FROM "postprocessings" WHERE "postprocessings"."device_id" = $1 LIMIT $2  [["device_id", 16226], ["LIMIT", 1]]

I think we simply merge this PR (after your advice on the deprecation warning from https://github.com/fablabbcn/smartcitizen-api/pull/257#issuecomment-1742518022), but we potentially can use the Refactor for improving these type of loads, also on the Postprocessing part of the device too?

I am not an expert, but is there a way in which this relationship could be improved?

oscgonfer commented 1 year ago

Coming back to this topic. Based on a review of the documentation, these would be the desired parameters. I mark ~the ones to remove~ as well, in case you want to discuss. Also, I mark with (?) those that I don't see the point and finally, those that will be deprecated once we refactor (*):

Keys for devices

Key
id
name
description
owner_username
mac_address (only_admin)
owner_id
created_at
updated_at
last_recorded_at
state
exposure
tag_name
postprocessing_id
~hardware_info~ (see separate issue
kit_id (*)
geohash (?)
uuid
token (only_admin)

Keys for users

Key
id
username
~email~
created_at
updated_at
city
country_code
~url~
~avatar_url~
uuid

Sensors

Key
id
name
description
unit
measurement_id
ancestry
created_at
updated_at
uuid

Postprocessing

Key
blueprint_url
created_at
device_id
forwarding_params
hardware_url
id
latest_postprocessing
meta
updated_at

Tags

Key
created_at
description
id
name
updated_at
uuid
timcowlishaw commented 1 year ago

Hey hey Oscar! I've limited the ransackable paramaters to those you specified (leaving in the ?s for now) and have fixed the deprecation warning!

oscgonfer commented 1 year ago

Thanks @timcowlishaw we may have to discuss what to do with this merge in detail. For now I won't merge until we discuss, but some things to review:

timcowlishaw commented 11 months ago

Hey Oscar, just pushing an update to deal with the last few things, and make sure all the parameters above are tested properly (with a couple of exceptions I'll outline below).

Querying for a disallowed parameter will also now raise a 400 bad request with an error message which explains the error, which i think is probably the most appropriate response.

Let's think about the denial of service problem at a later date - I don't think any of these queries should be too heavy on their own, and the rate limiting should already help with multiple queries, but there's possibly cases I haven't considered.

Two things I haven't done:

1) Searching directly for postprocessings / tags - Devices can be queried by all the fields above, but the /postprocessings and /tags endpoints currently don't have ransack search set up. We can do this but it's a bigger job, and probably worth saving til after we've merged the refactor.

2) Devices can't be searched by 'exposure', which mystified me until i realised that 'exposure' isn't actually a database column, it's a field in a json metadata blob, which ransack can't search. We could move this out to its own column to make it searchable, but i think, again, it'd be worth doing that after the refactor!

Let's discuss next week!

Thanks,

Tim

oscgonfer commented 11 months ago

Hi!

Searching directly for postprocessings / tags - Devices can be queried by all the fields above, but the /postprocessings and /tags endpoints currently don't have ransack search set up. We can do this but it's a bigger job, and probably worth saving til after we've merged the refactor.

Devices can't be searched by 'exposure', which mystified me until i realised that 'exposure' isn't actually a database column, it's a field in a json metadata blob, which ransack can't search. We could move this out to its own column to make it searchable, but i think, again, it'd be worth doing that after the refactor!

Totally agree. We only need to keep this one for now in devices that is related to postprocessing:

https://staging-api.smartcitizen.me/v0/devices?q[postprocessing_id_not_null]=1

Let's think about the denial of service problem at a later date - I don't think any of these queries should be too heavy on their own, and the rate limiting should already help with multiple queries, but there's possibly cases I haven't considered.

Agree. Tests below work well now as pagination returns data properly now. This doesn't seem to be an issue anymore.

Finally: Now this should be deployed in staging for testing. I did some random tests, but maybe if we do them more "methodically" we might be able to uncover some side issues. Maybe we should prepare a matrix of combinations of different queries for further testing.

timcowlishaw commented 11 months ago

Hey oscar, I've fixed the 'no error returned on 400' problem, so this should be ready to go when i come in later!