Skip to content

Conversation

@scortier
Copy link
Member

@scortier scortier commented Mar 21, 2022

Added filtering by certain fields in assets api.

Querystring: "?types=table&services=bigquery&size=30&offset=50&sort=created_at&direction=desc&data[dataset]=booking&data[project]=p-godata-id&q=internal&q_fields=name,urn,description,services",

  • filter by types : types=topic,table
  • filter by services: services=kafka,postgres
  • filter by field in asset.data: data[dataset]=booking&data[project]=p-godata-id"
  • querying by field ( includes by name , urn): q=internal&q_fields=name,urn,description,services
  • sort by certain fields : sort=created_at
  • sorting direction (asc / desc) : direction=desc

@scortier scortier marked this pull request as draft March 21, 2022 15:23
@scortier scortier marked this pull request as ready for review March 29, 2022 19:03
@scortier scortier requested review from StewartJingga and mabdh March 29, 2022 19:08
@scortier scortier self-assigned this Mar 29, 2022
Copy link
Contributor

@StewartJingga StewartJingga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also we need to update AssetRepository.GetCount to also read and process the same filter config as GetAll. Or else we will get wrong total value.

@mabdh mabdh linked an issue Mar 31, 2022 that may be closed by this pull request
@mabdh
Copy link
Member

mabdh commented Mar 31, 2022

@scortier

filter by field in asset.data: config.data[dataset]=booking&config.data[project]=p-godata-id"

If it is for filtering, why not using filter prefix instead?


The nested query params is in the wrong format.
config.data[dataset]=booking&config.data[project]=p-godata-id"
config[data.dataset]=booking&config[data.project]=p-godata-id"
filter[data.dataset]=booking&filter[data.project]=p-godata-id" ✅ ✅

cc @StewartJingga

@scortier
Copy link
Member Author

scortier commented Mar 31, 2022

@scortier

filter by field in asset.data: config.data[dataset]=booking&config.data[project]=p-godata-id"

If it is for filtering, why not using filter prefix instead?

The nested query params is in the wrong format. config.data[dataset]=booking&config.data[project]=p-godata-id"config[data.dataset]=booking&config[data.project]=p-godata-id"filter[data.dataset]=booking&filter[data.project]=p-godata-id" ✅ ✅

cc @StewartJingga

Yes, we can go with the filter prefix, no issue with that. Regarding query param, while discussion with @StewartJingga finalised the 1st query param approach, but it seems last option seems more preferable as per requirement; updating the query param format to last option.

@scortier
Copy link
Member Author

scortier commented Apr 1, 2022

@mabdh @StewartJingga, the proto file needs to be updated to pass correct config in GetAllAsset method in v1beta1/asset, due to this reason the test is failing, shall we update the proto file and fix this test or move forward with failing test, as fixing the proto file is in scope of this issue.
WDYT ?

@StewartJingga
Copy link
Contributor

@scortier

filter by field in asset.data: config.data[dataset]=booking&config.data[project]=p-godata-id"

If it is for filtering, why not using filter prefix instead?

The nested query params is in the wrong format. config.data[dataset]=booking&config.data[project]=p-godata-id"config[data.dataset]=booking&config[data.project]=p-godata-id"filter[data.dataset]=booking&filter[data.project]=p-godata-id" ✅ ✅

cc @StewartJingga

Why there is config there? We can directly use data[] instead. @scortier

@StewartJingga
Copy link
Contributor

@mabdh @StewartJingga, the proto file needs to be updated to pass correct config in GetAllAsset method in v1beta1/asset, due to this reason the test is failing, shall we update the proto file and fix this test or move forward with failing test, as fixing the proto file is in scope of this issue. WDYT ?

We can just fix the proto in this PR instead, we dont want breaking test on main branch. @scortier

@scortier
Copy link
Member Author

scortier commented Apr 1, 2022

@scortier

filter by field in asset.data: config.data[dataset]=booking&config.data[project]=p-godata-id"

If it is for filtering, why not using filter prefix instead?
The nested query params is in the wrong format. config.data[dataset]=booking&config.data[project]=p-godata-id"config[data.dataset]=booking&config[data.project]=p-godata-id"filter[data.dataset]=booking&filter[data.project]=p-godata-id" ✅ ✅
cc @StewartJingga

Why there is config there? We can directly use data[] instead. @scortier

Recently, updated the query param to this format : filter[data.dataset]=booking&filter[data.project]=p-godata-id", should we update it to data[dataset]=booking&data[project]=p-godata-id" ?, but we discuss earlier the format should be of type filter[{fields}]. @mabdh @StewartJingga

@mabdh
Copy link
Member

mabdh commented Apr 1, 2022

For the proto, in that case I think this PR could also covers this issue #105 @scortier @StewartJingga

or as an alternative, please create different AssetConfig struct for grpc and http. Later in next issue we can unify it.

@scortier scortier requested review from StewartJingga and mabdh April 1, 2022 07:14
Copy link
Contributor

@StewartJingga StewartJingga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@StewartJingga StewartJingga merged commit a88b6fe into main Apr 1, 2022
@StewartJingga StewartJingga deleted the assets-filters branch April 1, 2022 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: add filtering in assets api

4 participants