Validate sort order in admin.ResourceListRequest

[iaroslav-ciupin]

TL;DR

Validate sort order by checking it against supported columns. This PR only checks sort order in admin.ResourceListRequest, other requests will be added in separate PR. Also validation for filters will be added in separate PR.

Type

• Bug Fix
• Feature
• Plugin

Are all requirements met?

• Code completed
• Smoke tested
• Unit tests added
• Code documentation added
• Any pending items have an associated Issue

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

Remove the 'fixes' keyword if there will be multiple PRs to fix the linked issue

fixes https://github.com/flyteorg/flyte/issues/

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

[codecov]

Codecov Report

Merging #606 (4683b91) into master (d492640) will increase coverage by 1.61%.
Report is 1 commits behind head on master.
The diff coverage is 70.39%.

❗ Current head 4683b91 differs from pull request most recent head d8991be. Consider uploading reports for the commit d8991be to get more accurate results

@@            Coverage Diff             @@
##           master     #606      +/-   ##
==========================================
+ Coverage   58.68%   60.30%   +1.61%     
==========================================
  Files         171      171              
  Lines       16484    13454    -3030     
==========================================
- Hits         9674     8113    -1561     
+ Misses       5958     4488    -1470     
- Partials      852      853       +1     

Flag	Coverage Δ
unittests	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
pkg/manager/impl/named_entity_manager.go	41.66% <0.00%> (+2.38%)	⬆️
...g/repositories/gormimpl/description_entity_repo.go	69.49% <0.00%> (+3.66%)	⬆️
pkg/repositories/gormimpl/signal_repo.go	73.58% <0.00%> (+4.46%)	⬆️
pkg/repositories/gormimpl/task_execution_repo.go	64.35% <0.00%> (-0.22%)	⬇️
pkg/manager/impl/execution_manager.go	73.06% <50.00%> (+2.48%)	⬆️
pkg/repositories/gormimpl/launch_plan_repo.go	65.04% <50.00%> (+1.13%)	⬆️
pkg/repositories/gormimpl/task_repo.go	72.28% <50.00%> (+4.01%)	⬆️
pkg/repositories/gormimpl/workflow_repo.go	70.12% <50.00%> (+2.08%)	⬆️
pkg/repositories/gormimpl/common.go	66.66% <60.00%> (+3.80%)	⬆️
pkg/manager/impl/launch_plan_manager.go	63.73% <71.42%> (+2.65%)	⬆️
... and 12 more

... and 136 files with indirect coverage changes

[katrogan]

Because we join on multiple tables for the list request we should support sort columns from any of the supported tables

See https://docs.flyte.org/en/latest/concepts/admin.html#putting-it-all-together for how we document table_name.field as an option sort

[iaroslav-ciupin]

@katrogan from what I see from code and docs, we are joining with tables references in filters, not in sort_by.
I was able to successfully perform this query https://dogfood.cloud-staging.union.ai/api/v1/executions/flytesnacks/development?limit=5&filters=eq(workflow.name,ray_example.ray_example.ray_workflow)

But I got an error for
https://dogfood.cloud-staging.union.ai/api/v1/executions/flytesnacks/development?limit=5&sort_by.key=workflow.name&filters=eq(workflow.name,ray_example.ray_example.ray_workflow)
and
https://dogfood.cloud-staging.union.ai/api/v1/executions/flytesnacks/development?limit=5&sort_by.key=workflow.name
Error

{
  "error": "cannot query with specified table attributes: missing FROM-clause entry for table \"workflow\"",
  "code": 3,
  "message": "cannot query with specified table attributes: missing FROM-clause entry for table \"workflow\""
}

[katrogan]

thanks for the verification @iaroslav-ciupin
do you mind filing a follow-up issue to ensure that we update JoinTableEntities to include other tables when they're included in the sort key as well?

[katrogan]

Actually, can you still ensure that we pass in the join table columns here as allowed columns?

[iaroslav-ciupin]

@katrogan I don't get, why? we currently don't support sorting by join table columns and return error containing database schema details which is not a good thing. Wouldn't it be better to return a clearer validation error ?

[katrogan]

ah I see - missed your second example, so even for the successful join call, e.g.
/api/v1/executions/flytesnacks/development?limit=5&filters=eq(workflow.name,ray_example.ray_example.ray_workflow)&sort_by.key=workflow.created_at&sort_by.direction=DESCENDING

we still fail to sort by so we're not removing functionality in this change

[iaroslav-ciupin]

@katrogan exactly, although we specify join table column in filter and we still can't sort by join table column

[katrogan]

Actually, can you still ensure that we pass in the join table columns here as allowed columns?