privacysandbox / protected-auction-key-value-service

Protected Auction Key/Value Service
Apache License 2.0
53 stars 20 forks source link

V1 => V2 adapter doesn't parse multiple keys #52

Closed fhoering closed 4 months ago

fhoering commented 4 months ago

We are deploying the v2 version of the the key/value server which seems the default now and which means the UDFs need to reply in a dedicated format (as indicated here) In order to be able to do some simple load tests and for Chrome to be able to call the key/value server which existing our interest groups we actually need to activate the v1 version. It seems like the v1 => v2 adapter doesn't properly parse the keys. In the example below key1,key2 is sent as ['key1,key2'] instead of ['key1', 'key2']

$ curl http://localhost:51052/v1/getvalues?keys=key
{
 "keys": {
  "key": {
   "value": "reply for key"
  }
 },
 "renderUrls": {},
 "adComponentRenderUrls": {},
 "kvInternal": {}
}
$ curl http://localhost:51052/v1/getvalues?keys=key,key1
{
 "keys": {
  "key,key1": {
   "value": "reply for key,key1"
  }
 },
 "renderUrls": {},
 "adComponentRenderUrls": {},
 "kvInternal": {}
}

It seems like in this case handler GetKeys is never called. I fixed it with this commit: https://github.com/fhoering/protected-auction-key-value-service/commit/3de03e57d2b065a670200008d6249c354fde5c90

It would be nice to be able to fully run the CI from github and/or also have a doc on how to run the tests locally.

Tested with:

curl http://localhost:51052/v1/getvalues?keys=key,key1
{
 "keys": {
  "key1": {
   "value": "reply for key1"
  },
  "key": {
   "value": "reply for key"
  }
 },
 "renderUrls": {},
 "adComponentRenderUrls": {},
 "kvInternal": {}
}
curl http://localhost:51052/v1/getvalues?keys=key
{
 "keys": {
  "key": {
   "value": "reply for key"
  }
 },
 "renderUrls": {},
 "adComponentRenderUrls": {},
 "kvInternal": {}
}
lx3-g commented 4 months ago

Thnx for the fix! I've just merged your PR.

fhoering commented 4 months ago

Ok. Thanks.

Two additional questions.

What is the command to execute the tests locally actually ? I didn't find it easily and didn't get to work bazel build //components/data_server/server:server --//:platform=local --//:instance=local

Do you think tests can be executed inside github actions also ?

lx3-g commented 4 months ago

Do you think tests can be executed inside github actions also ?

This is a feature request that we are considering, and we will update you once there is any decision or action there.

What is the command to execute the tests locally actually ?

Just to be clear, do you mean unit tests?

fhoering commented 4 months ago

Yes unit tests. I actually tried bazel test as the build works: `bazel test //components/data_server/server:server --//:platform=local --//:instance=local``

lx3-g commented 4 months ago

//components/data_server/server:server is a binary. You should be running test against cc_test targets. E.g. `bazel test //components/data_server/server:server_lib_test --//:platform=local --//:instance=local``

fhoering commented 4 months ago

OK. I close this ticket. If you need one to request running the tests in github I can also open a new one. Thanks