Css 10530/filterning resources #1353
Conversation
internal/jujuapi/controllerroot.go
Outdated
| @@ -66,7 +66,7 @@ type JIMM interface { | |||
| InitiateMigration(ctx context.Context, user *openfga.User, spec jujuparams.MigrationSpec) (jujuparams.InitiateMigrationResult, error) | |||
| ListApplicationOffers(ctx context.Context, user *openfga.User, filters ...jujuparams.OfferFilter) ([]jujuparams.ApplicationOfferAdminDetailsV5, error) | |||
| ListIdentities(ctx context.Context, user *openfga.User, filter pagination.LimitOffsetPagination) ([]openfga.User, error) | |||
| ListResources(ctx context.Context, user *openfga.User, filter pagination.LimitOffsetPagination) ([]db.Resource, error) | |||
| ListResources(ctx context.Context, user *openfga.User, filter pagination.LimitOffsetPagination, nameFilter, typeFilter string) ([]db.Resource, error) | |||
There was a problem hiding this comment.
What do you think about using stronger types for nameFilter and typeFilter that provide validation?
Like
type NameFilter struct{
name string
}Then we can have
func (n NameFilter) Validate() bool{}There was a problem hiding this comment.
the thing is that validation is only for entity and it should be done at API layer before entering Jimm world.
I'll be constructing a strong just for one of the two, so i don't see many benefits.
But I am open to be convinced
SimoneDutto
force-pushed
the
CSS-10530/filterning-resources
branch
from
7e380fa to
08afc6e
Compare
internal/db/resource.go
Outdated
|
|
||
| applicationOffersQuery := db.Select(selectApplicationOffers). | ||
| Model(&dbmodel.ApplicationOffer{}). | ||
| Where("? IS TRUE OR application_offers.name LIKE ?", namePrefixEmpty, namePrefixFilter). |
There was a problem hiding this comment.
Thanks for applying this suggestion, but I felt a bit suspicious about PostgreSQL expression evaluation (and short-circuiting of condition), and found out there is no such guaranty in PostgreSQL (See here). The gist is we have to use the CASE statement. With that we don't need the extra boolean argument:
WHERE (CASE WHEN ? = '' THEN TRUE ELSE application_offers.name LIKE ? END)I tried this with PostgreSQL and it seems working.
There was a problem hiding this comment.
i was not aware of that.
Updated
| func buildQuery(db *gorm.DB, offset, limit int, namePrefixFilter, typeFilter string) (*gorm.DB, error) { | ||
| applicationOffersQuery := db.Select(selectApplicationOffers). | ||
| Model(&dbmodel.ApplicationOffer{}). | ||
| Where("(CASE WHEN ? = '' THEN TRUE ELSE application_offers.name LIKE ? END)", namePrefixFilter, namePrefixFilter+"%"). |
There was a problem hiding this comment.
| Where("(CASE WHEN ? = '' THEN TRUE ELSE application_offers.name LIKE ? END)", namePrefixFilter, namePrefixFilter+"%"). | |
| Where("(CASE WHEN ? = '' THEN TRUE ELSE application_offers.name LIKE ?% END)", namePrefixFilter, namePrefixFilter). |
? and elsewhere
There was a problem hiding this comment.
it doesn't work with the SQL replacements
| namePrefixFilter, typeFilter := utils.GetNameAndTypeResourceFilter(params.EntityName, params.EntityType) | ||
| if typeFilter != "" { | ||
| typeFilter, err = validateAndConvertResourceFilter(typeFilter) | ||
| if err != nil { | ||
| return nil, v1.NewInvalidRequestError(err.Error()) | ||
| } | ||
| } |
There was a problem hiding this comment.
For a follow-up, but I think this should be calling an application layer function to create custom types. Like the idea I was suggesting earlier, the API layer can pass the strings it has received from the client into application layer validators that return app layer types that can be used in ListResources().
There was a problem hiding this comment.
Because a string doesn't tell me much, but a type resource struct{ resource string } tells me a lot more.
Description
Follow-up to 1347 to enable filtering based of resource name, resource type.
Fixes JIRA/GitHub issue number
Engineering checklist
Check only items that apply
Test instructions
Notes for code reviewers