Skip to content

Conversation

@javirln
Copy link
Member

@javirln javirln commented Mar 11, 2025

This patch allows to filter Workflows using a JSON filter. A JSON filter is a filter that works over JSON data in a specific column. Such data shall be stored in the database.

The JSON filter is later converted into a valid SQL statement.

@javirln javirln requested review from jiparis and migmartri March 11, 2025 15:44
@javirln javirln self-assigned this Mar 11, 2025
const columnNotFoundSQLState = "42703"

// columnNotExistRegex extracts column name from the error message
var columnNotExistRegex = regexp.MustCompile(`column "([^"]+)" does not exist`)
Copy link
Member

Choose a reason for hiding this comment

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

I'd not support dynamic selection of columns, that's an implementation detail to me that should not be exposed in then API

Copy link
Member

@migmartri migmartri left a comment

Choose a reason for hiding this comment

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

Thanks @javirln, my take on this implementation

  • I'd not allow dynamically supporting column selection, that's an implementation details that I do not think it should get exposed at the API layer, nevertheless I am scared that it could be a threat ventor (i.e crafting to query columns that you are not supposed to query). Instead, I'd let the data layer to decide what column the filter must be applied to.
  • In the name of Yagni (which I can be quite opinionated sometimes), I'd not add any other code than the one we are going to use. This means, no more selectors than the one that we plan to use, no code is the best code.

Thanks!

javirln added 4 commits March 24, 2025 19:48
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
@javirln
Copy link
Member Author

javirln commented Mar 24, 2025

The record on the database has a json value on the metadata column with the following information:

{"random_key": "random_value"}

Examples:

Filtering the value successfully:

$ grpcurl -plaintext  -H "Authorization: Bearer $LOCAL_TOKEN" -d '{"json_filters": [{"field_path": "random_key", "operator": "JSON_OPERATOR_EQ", "value": "random_value"}]}' localhost:9000 controlplane.v1.WorkflowService.List

{
  "result": [
    {
      "id": "a6673a04-c107-4f3e-bdc5-b35954aa9900",
      "name": "build-package",
      "project": "core",
      "createdAt": "2025-03-11T12:21:47.433740Z",
      "contractName": "build-package-contract",
      "contractRevisionLatest": 1
    }
  ],
  "pagination": {
    "page": 1,
    "pageSize": 50,
    "totalCount": 1,
    "totalPages": 1
  }
}

Change the operator so, no entry found:

$ grpcurl -plaintext  -H "Authorization: Bearer $LOCAL_TOKEN" -d '{"json_filters": [{"field_path": "random_key", "operator": "JSON_OPERATOR_NEQ", "value": "random_value"}]}' localhost:9000 controlplane.v1.WorkflowService.List

{
  "pagination": {
    "page": 1,
    "pageSize": 50
  }
}

Multiple filters applied are combined using an AND. Applying the EQ and NEQ filters, result no entries found

$ grpcurl -plaintext  -H "Authorization: Bearer $LOCAL_TOKEN" -d '{"json_filters": [{"field_path": "random_key", "operator": "JSON_OPERATOR_EQ", "value": "random_value"}, {"field_path": "random_key", "operator": "JSON_OPERATOR_NEQ", "value": "random_value"}]}' localhost:9000 controlplane.v1.WorkflowService.List

{
  "pagination": {
    "page": 1,
    "pageSize": 50
  }
}

Copy link
Member

@jiparis jiparis left a comment

Choose a reason for hiding this comment

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

Awesome, @javirln . Just one nit comment about the operator verbosity.
Thanks!

predicates = append(predicates, jsonPredicate)
}
// Combine the predicates using OR logic
selector.Where(sql.Or(predicates...))
Copy link
Member

Choose a reason for hiding this comment

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

interestingly, you chose to make or the default; it feels counterintuitive, no?

I mean if you provide multiple filters, my fist thought would be to become an AND. What do you think?

Copy link
Member Author

@javirln javirln Mar 25, 2025

Choose a reason for hiding this comment

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

You might be right, and I was just biassed by another PR. I've changed it to merge all filters in an AND. Thanks.

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
@javirln javirln merged commit 3d6884e into chainloop-dev:main Mar 25, 2025
13 checks passed
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.

3 participants