Skip to content
This repository has been archived by the owner on Jun 28, 2024. It is now read-only.

feat: add functions to automatically navigate device pages #135

Closed
wants to merge 1 commit into from

Conversation

ctreatma
Copy link
Contributor

@ctreatma ctreatma commented Aug 30, 2023

This serves as an example of how we could implement automatic page navigation, similar to what was done in packngo.

Relates to #131

This serves as an example of how we could implement automatic page
navigation, similar to what was done in `packngo`.
@@ -0,0 +1,58 @@
diff --git a/metal/v1/api_devices.go b/metal/v1/api_devices.go
Copy link
Member

@displague displague Aug 31, 2023

Choose a reason for hiding this comment

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

If we were to maintain local copies of the Go templates, we could update the template files whenever we update the OpenAPITools version via the meta CLI command which dumps the templates. In that case, is this where we would patch in a generic (mustache) version of the paginated executor:

https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/go/api.mustache#L77-L79

We would need a mustache way to identify if pagination is supported in the resource, preferably without needing to modify the Java defined attributes.

Copy link
Member

@displague displague Aug 31, 2023

Choose a reason for hiding this comment

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

Finding a generic approach will be helpful for templating our generated Go pattern across Fabric, NE, and future Go SDKs.

Copy link
Member

Choose a reason for hiding this comment

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

For Metal, if this is safe for unpaginated endpoints, perhaps we can always include ExecuteWithPagination and it will degrade to a less efficient Execute:

if page.Meta.GetLastPage() <= page.Meta.GetCurrentPage() {

Copy link
Member

@displague displague Aug 31, 2023

Choose a reason for hiding this comment

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

type Paginated interface {
  GetCurrentPage() int32
  GetLastPage() int32
}

type WithMeta interface {
  HasMeta() bool // may just need this, if it exists
  GetMeta() something // otherwise this
}
...
if meta, ok := page.Meta.(Paginated); meta != nil && ok { 
  // ^ check meta first? does meta always exist? nil? A WithMeta Interface check could be used if this simple check wouldn't suffice
}

index 2dc180a..5191167 100644
--- a/metal/v1/api_devices.go
+++ b/metal/v1/api_devices.go
@@ -2034,6 +2034,26 @@ func (r ApiFindOrganizationDevicesRequest) Execute() (*DeviceList, *http.Respons
Copy link
Member

Choose a reason for hiding this comment

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

This is a significant patch context. Chasing func (r ApiFindOrganizationDevicesRequest) Execute() will keep our patches applying safely. 👍

@t0mk
Copy link
Contributor

t0mk commented Aug 31, 2023

@displague @ctreatma
I have tried to implement pagination with the mustache templates of openapi generator today. there are several issues:

  • Mustache is logic-less, so it's hard to implement the condition of when the paginated-list function should be emitted by the generator. There's nothing like if parameters[].name == "page". the furthest I got was to iterate through query params of a get-request operation, but even then I wasn't able to condition properly. It seems it's by design:
    https://stackoverflow.com/questions/20345288/check-with-mustache-js-if-parameter-is-a-specific-value

  • Then, I was thinking I would add a boolean attribute isPaginated to ['paths'][/project/{id}/devices][get] in the spec. That would allow to distinguish in the mustache templates, but it makes the spec not valid.

This was my template that would generate files like api_device_paginated_lister.go.

templates/api_paginated_lister.mustache:

{{>partial_header}}
package {{packageName}}

{{#operations}}
import (
	"bytes"
	"context"
	"io"
	"net/http"
	"net/url"
{{#imports}}	"{{import}}"
{{/imports}}
)

{{#operation}}

{{isPaginated}}

// The code is just a placeholder, I wanted to know if it's emitted only on paginated methods

func (a *{{{classname}}}Service) {{nickname}}Execute(r {{#structPrefix}}{{&classname}}{{/structPrefix}}{{^structPrefix}}Api{{/structPrefix}}{{operationId}}Request) ({{#returnType}}{{^isArray}}{{^returnTypeIsPrimitive}}{{^isResponseFile}}*{{/isResponseFile}}{{/returnTypeIsPrimitive}}{{/isArray}}{{{.}}}, {{/returnType}}*http.Response, error) {

	var items []{{returnType}}

	pageNumber := int32(1)

	// classname {{classname}}, {{&classname}}


	for {
		page, _, err := r.Page(pageNumber).Execute()
		if err != nil {
			return nil, err
		}

		items = append(items, page.Devices...)
		if page.Meta.GetLastPage() <= page.Meta.GetCurrentPage() {
			break
		}
	}

	return items, nil
}
{{/isPaginated}}

{{/operation}}
{{/operations}}

To consider the templates, we must have config.yaml as

templateDir: /local/templates
files:
  api_paginated_lister.mustache:
    templateType: API
    destinationFilename: _paginated_lister.go

.. and -c config.yaml to the openapi-cli call.

@t0mk
Copy link
Contributor

t0mk commented Aug 31, 2023

I also tried to come up with an yq expression that would add a boolean flag isPaginated to ["paths"]["/projects{id}/devices"]["get"]. I came up with

yq 'with(.paths[].get | select(.parameters[].name == "page"); .isPaginated=true)' < $1 > $2

.. but it's not completely correct.

@displague
Copy link
Member

@t0mk The Path syntax is well defined, you can't add arbitrary fields unless they follow the x- format.

@displague
Copy link
Member

@t0mk page.Devices would also need to be template-ified in your example. I don't presume determining the proper plural is trivial.

@t0mk
Copy link
Contributor

t0mk commented Sep 11, 2023

@displague Thanks for the tip on the x- properties, it couldn't a fix for my problem.
https://swagger.io/specification/#paths-object

As for the the plural, I think it can be worked out from the template variables. It might not be gramtatically correct, but I don't think it will conflict.

@ctreatma
Copy link
Contributor Author

Closing in favor of #150, which will generate the changes in this PR for all paginated endpoints using a combination of vendor extensions and custom templates.

@ctreatma ctreatma closed this Sep 20, 2023
t0mk added a commit that referenced this pull request Sep 27, 2023
This PR adds logic for generating listing functions capable of going
through all the pages (think devices in a project, or events in a
device).

It adds templates for the new code, docs, and code tests. The templates
are fetched from oag v7.0.0, and then modified for the sake of the
paginated listers.

It uses vendor extension property object in the form
```yaml
     x-paginated:
        x-paginated-property: Events
```

The spec patching part can be done in other oag generated SDKs.

related to #135 

fixes #131

---------

Signed-off-by: Tomáš Karásek <t0mk@users.noreply.github.com>
Co-authored-by: Marques Johansson <mjohansson@equinix.com>
@ctreatma ctreatma deleted the auto-pagination branch November 1, 2023 17:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants