elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.73k stars 8.14k forks source link

API Standardization around Paging #135490

Open pickypg opened 2 years ago

pickypg commented 2 years ago

Describe the feature:

APIs in Kibana are implementing paging individually, rather than sharing the logic of parsing the paging parameters to force them to share names and formats.

/api/detection_engine/_find?per_page=100:

https://github.com/elastic/kibana/blob/414ad78ffa29753868bfeacc59ab17bedb1c0467/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/find_rules_route.ts#L55

/api/fleet/agent_policies?perPage=100:

https://github.com/elastic/kibana/blob/fd3bb166c6ce2bfbe3cfa85fb8e97d2fc42c16a5/x-pack/plugins/fleet/server/services/agent_policy.ts#L286 https://github.com/elastic/kibana/blob/fd3bb166c6ce2bfbe3cfa85fb8e97d2fc42c16a5/x-pack/plugins/fleet/common/openapi/paths/agent_policies.yaml#L20-L21

per_page seems to be the more common form of the parameter.

Describe a specific use case for the feature:

Paging, and other equivalent parameters across Kibana's APIs (e.g., sorting), should use the same names and formats unless there's a strong reason not to do so. Ideally, this could be done in a non-breaking way that picks one name and deprecates the other, but parses both for every pageable API.

elasticmachine commented 2 years ago

Pinging @elastic/fleet (Team:Fleet)

elasticmachine commented 2 years ago

Pinging @elastic/security-solution (Team: SecuritySolution)

jsanz commented 2 years ago

Thanks @pickypg for raising this issue. I'm tagging the teams responsible of the code you posted. I guess this is a broader problem across Kibana but I'll leave to them to decide how to move forward.

jen-huang commented 2 years ago

Should we discussing standardization for query parameters in general, not just pagination?

Fleet uses camelCase for our parameters: https://github.com/elastic/kibana/blob/4a82f9815f9278e4a4a6bfd54118789dd4ed012c/x-pack/plugins/fleet/server/types/rest_spec/common.ts#L11

I believe this decision was mostly driven by our linting rules, avoiding having to rename snake_case vars to camelCase in development:

image

cc @kpollich

pickypg commented 2 years ago

Should we discussing standardization for query parameters in general, not just pagination?

Definitely yes, but pagination is probably the most truly common base across Kibana and probably an easier place to start the discussion. But there should be a unification across Kibana about snake_case versus camelCase.

111andre111 commented 2 years ago

By the way there is as well another parameter on /api/endpoint/metadata called pageSize: https://github.com/elastic/kibana/blob/decdafab3121de2272bfc00782615b93dd31e89c/x-pack/plugins/security_solution/common/endpoint/types/index.ts#L183-L194

paul-tavares commented 2 years ago

FYI - within my team (Security Solution Endpoint Onboarding and lifecycle management), we had discussed this same topic a few months ago as some of our own APIs started to have very different implementations. We agreed on the following, which we have now been trying to follow and refactor older APIs to the new url query params:

// List API request:
{
    page: number;
    pageSize: number;
    sort: string;
    sortOrder: ‘asc’ | ‘desc’;
}

// List API response (at a minimum):
{
    page: number;
    pageSize: number;
    total: number;
    data: any; // API's main payload
}

// Details API response
{
    data: any // API's main payload
}

In some cases, other attributes may be added to the response - more likely in cases where it needs to return enriched data that complement the main payload. The List request could also have other query params as needed - ex. kuery, filter, etc...

/paul

cc/ @kevinlog , @pzl, @joeypoon