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

Don't use the same Filter for multiple messages (proto) #1053

Closed
lebogg opened this issue Apr 5, 2023 · 5 comments · Fixed by #1480
Closed

Don't use the same Filter for multiple messages (proto) #1053

lebogg opened this issue Apr 5, 2023 · 5 comments · Fixed by #1480
Labels
api bug Something isn't working

Comments

@lebogg
Copy link
Member

lebogg commented Apr 5, 2023

message Filter {
// Optional. List only assessment results of a specific cloud service.
optional string cloud_service_id = 1 [ (validate.rules).string.uuid = true ];
// Optional. List only compliant assessment results.
optional bool compliant = 2;
// Optional. List only assessment results of a specific metric id.
repeated string metric_ids = 3
[ (validate.rules).repeated .items.string.min_len = 1 ];
optional string metric_id = 4 [ (validate.rules).string.min_len = 1 ];
}

Here, the Filter is used for both ListAssessmentResultsRequest and ListAssessmentToolsRequest. I guess the fields are not appropriate for both cases? In addition, the comments refer only to assessment results.

@lebogg lebogg added api bug Something isn't working labels Apr 5, 2023
@oxisto
Copy link
Member

oxisto commented Apr 5, 2023

Ah, it seems all the filter of the service were merged into a single one. Looks like we need distinct filter objects and then the question is: Is that worth it? Or would it be better just to have a common handling of the filter fields in the service implementation?

@oxisto
Copy link
Member

oxisto commented Apr 7, 2023

We could also embed the filter type directly into the message like this:

message ListResourcesRequest {
  message Filter {
    optional string type = 1;
    optional string cloud_service_id = 2;
  }

  optional Filter filter = 1;

  int32 page_size = 10;
  string page_token = 11;
  string order_by = 12;
  bool asc = 13;
}

although this creates a type called ListResourcesRequest_Filter in Go.

Probably renaming the type to ListResourcesFilter would be cleaner?

message ListResourcesRequest {
  optional ListResourcesFilter filter = 1;

  int32 page_size = 10;
  string page_token = 11;
  string order_by = 12;
  bool asc = 13;
}

message ListResourcesFilter {
  optional string type = 1;
  optional string cloud_service_id = 2;
}

or ListResourcesRequestFilter?

cc @lebogg @luis-gasparschroeder

@anatheka
Copy link
Member

I like more the upper one with the filter embedded in the message.

@lebogg
Copy link
Member Author

lebogg commented Apr 11, 2023

Ah, it seems all the filter of the service were merged into a single one. Looks like we need distinct filter objects and then the question is: Is that worth it? Or would it be better just to have a common handling of the filter fields in the service implementation?

Tricky question. I kinda dislike one filter for multiple requests. But, on the other side, if we have one such big filter and comment appropriately to which Requests they belong and also do the handling of these filters in the implementation (like you mentioned), it could work.

Otherwise, I would prefer the embedded option in the case of single filters.

@oxisto
Copy link
Member

oxisto commented Jun 20, 2023

We need to check, if the "new" approach (unique filter message in request) is used everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants