Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add filtering in assets api #100

Merged
merged 23 commits into from
Apr 1, 2022
Merged

feat: add filtering in assets api #100

merged 23 commits into from
Apr 1, 2022

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 mabdh and StewartJingga 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
3 participants