feat(go): template improvements #1444

mehmetaligok · 2023-04-04T11:33:20Z

🧭 What and Why

🎟 JIRA Ticket: https://algolia.atlassian.net/browse/APIC-660

With #1423, the initial setup merged but as explained in that PR we need to make more improvements to finalize the generation.

This PR is meant the close the requirement gap on the go client. There is still some improvement room for the implementation but the needed requirements should be fulfilled with this PR.

Changes included:

templates/go/api.mustache updated to handle parameters properly.
- Optional parameters, precisely the optional body parameters, created problems with the generated code.
- It was converted to a struct approach to creating the correct structure for all request param. It is similar to the default go template but it is altered according to our needs.
templates/go/utils.mustache added to prevent code generation errors.
- The nullable primitive types are not generated automatically, with this file, all needed nullable types are added. As of now, we only need NullableBool but I didn't remove the rest since it may be needed in the future.
transport layer is added as the original client and the templates are updated to use the transport layer.
- It is mostly moved from algoliasearch-client-go and removed unnecessary pieces with preserving the functionality.
- There are some improvement points on the moved codes but it is better to do these improvements after we set the tests and when we are sure that the client is working as expected. There are some complex operations so moving and updating at the same time could bring some unnoticed bugs. This can prolong the development time which is needed to make this client ready. We can improve this internal layer without breaking changes later on.

🧪 Test

The playground example was briefly updated to ensure the new request structure is working. Also, I couldn't find any static check errors on the code after the changes.

netlify · 2023-04-04T11:33:26Z

✅ Deploy Preview for api-clients-automation ready!

Name	Link
🔨 Latest commit	`feef2aa`
🔍 Latest deploy log	https://app.netlify.com/sites/api-clients-automation/deploys/642d78df015ce70008130b25
😎 Deploy Preview	https://deploy-preview-1444--api-clients-automation.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

algolia-bot · 2023-04-04T11:34:03Z

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the main branch.
You can still access the code generated on main via this commit.

shortcuts · 2023-04-04T11:56:09Z

generators/src/main/java/com/algolia/codegen/AlgoliaGoGenerator.java

      additionalProperties.put("packageVersion", Utils.getClientConfigField("go", "packageVersion"));
+      // using hostForGo to avoid overriding host in supporting files
+      additionalProperties.put("hostForGo", (String) additionalProperties.get("host"));


does the go generator requires one more variable than the others? If not, I'd rather change the util so that all templates uses the same naming/logic than having a special case

for now it only requires this. My initial thought was also changing the util with other templates but I didn't want to do that directly. But if you are okay to do this change on other templates as well, I can do it. It should be fairly simple

Yep it's basically changing the variable we use, we should force consistency between generators where applicable so that we don't struggle maintaining

sure, I will update this with the next commit

millotp

looks good ! GG on bringing the retry strategy !

millotp · 2023-04-04T12:03:12Z

clients/algoliasearch-client-go/algolia/call/kind.go

+
+const (
+	Read Kind = iota
+	Write


isn't ReadWrite a third option ? for me the 3 are different

these constants are used with the operations, not for the hosts. They are used to call the API endpoints. We don't need another const as ReadWrite because there will be no usage, now. We are setting it as Read depending on the x-use-read-transporter and the rest of the time it is just Write. So there is no other option for now.

when defining hosts we are using the functions below and there are three of them as you mentioned. IsRead, IsWrite, and IsReadWrite.

clients/algoliasearch-client-go/algolia/debug/debug.go

clients/algoliasearch-client-go/algolia/internal/errs/no_more_host_to_try_err.go

clients/algoliasearch-client-go/algolia/internal/transport/requester.go

clients/algoliasearch-client-go/algolia/internal/transport/transport.go

millotp · 2023-04-04T12:09:29Z

clients/algoliasearch-client-go/algolia/internal/transport/transport.go

+	// Add Content-Encoding header, if needed
+	if shouldCompress(t.compression, req.Method, req.Body) {
+		switch t.compression {
+		case compression.GZIP:


do we plan on supporting other compression algorithm ? if not this could be an if.
also I don't think we do anything about gzip in other languages, maybe it's something we miss

Let me simplify this as you suggested.

--

tbh I am not sure about this compression stuff. It was supported by the original client and I thought adding it here will not break anything. But I couldn't see anything specific in the other generated clients.

millotp · 2023-04-04T12:11:28Z

templates/go/api.mustache

 }

-func WithPage(page int32) Option {
+func QueryParamOption(name string, val any) Option {


how is the DX with this change ? I know we shouldn't focus on that for the first iterations of the client, just wondering

At the current stage, this shouldn't be necessary at all because we are proving all optional and required parameters to the users. But in case someone sends a very specific header or query param I add this. Also, I added opts ...Option in case if it is needed.

Normally it will not affect the user but if it will be used in this stage it will look like this;

auths, err := ingestionClient.GetAuthentications( ingestionClient.NewApiGetAuthenticationsRequest().WithItemsPerPage(2), ingestion.QueryParamOption("myQueryParam1", "myQueryParamValue1"), ingestion.HeaderParamOption("myHeaderParam1", "myHeaderParamValue2"), )

the issue is that the user shouldn't have to worry if it's a query param or a header, it should be the job of the API client.
or is this just a custom fallback if the param is not in the api client ?

exactly this is this just a custom fallback if the param is not in the api client

all the query, header, or body params are provided to the users with the given specs. So normally they shouldn't worry about those but this is just a fallback functionality if there are some rare cases.

millotp · 2023-04-04T12:12:52Z

templates/go/api.mustache

 		{{#returnType}}
-		returnValue  {{^isArray}}{{^returnTypeIsPrimitive}}*{{/returnTypeIsPrimitive}}{{/isArray}}{{{.}}}
+		localVarReturnValue  {{^isArray}}{{^returnTypeIsPrimitive}}*{{/returnTypeIsPrimitive}}{{/isArray}}{{{.}}}


is there a globalVarReturnValue ? I don't think the distinction is necessary

These are made for providing extra safety when generating the code. Because using when with common var names there is a small colliding possibility.

For example; it was happening with body, in one of the specs we had a request with a body parameter, and the generated code was failing.

There is a small possibility for returnValue or postBody but still, it is better to reduce these chances as the default templates do.

I still think they hurt readability, if we know there is no collision with global variable we should remove it IMO

okay no huge deal for me.
I believe there is no collision as of now for returnValue and postBody. But let me double-check with generating the code to see.

alright, updated with feef2aa

millotp · 2023-04-04T12:13:18Z

templates/go/api.mustache

-  } else {
-  localVarPostBody = body
-  }
+  localVarPostBody = r.{{paramName}}


same here, it could just be postBody

millotp · 2023-04-04T13:21:07Z

oh also I noticed that the go client is using the default values in the models, we shouldn't use it and use nil instead, to leverage the default value of the API directly (because the API client might not be up-to-date)

mehmetaligok · 2023-04-04T13:38:09Z

oh also I noticed that the go client is using the default values in the models, we shouldn't use it and use nil instead, to leverage the default value of the API directly (because the API client might not be up-to-date)

@millotp Sorry, I couldn't fully understand. So this means we shouldn't utilize the default value on the specs in any of the models and if the value is not set by the user, we should marshal it as empty?

millotp · 2023-04-04T13:52:31Z

we shouldn't utilize the default value on the specs in any of the models and if the value is not set by the user, we should marshal it as empty

exactly, this is what we do in other languages, the default is purely informative for the user, but we don't rely on it

mehmetaligok · 2023-04-04T14:00:16Z

we shouldn't utilize the default value on the specs in any of the models and if the value is not set by the user, we should marshal it as empty

exactly, this is what we do in other languages, the default is purely informative for the user, but we don't rely on it

@millotp Oh alright, I haven't noticed this at all. I am not sure if the changes will be small or not. If you don't mind, I would like to tackle this with another PR. So we will not make this PR bigger.

mehmetaligok · 2023-04-04T15:37:49Z

we shouldn't utilize the default value on the specs in any of the models and if the value is not set by the user, we should marshal it as empty

exactly, this is what we do in other languages, the default is purely informative for the user, but we don't rely on it

@millotp Oh alright, I haven't noticed this at all. I am not sure if the changes will be small or not. If you don't mind, I would like to tackle this with another PR. So we will not make this PR bigger.

You can ignore this comment, it turned out pretty simple to do so I added it in this PR. When it is removed from the initialization function, the rest is automatically handled. If the user is not using specifically New{{param}}WithDefaults, we will not set the default values.

shortcuts

this looks super clean, I don't have anything blocking to add actually

shortcuts · 2023-04-05T13:03:33Z

clients/algoliasearch-client-go/algolia/debug/utils.go

+	return io.NopCloser(bytes.NewReader(data)), string(data)
+}
+
+func decodeGzipContent(in string) (string, error) {


do we expect to use this?

well, if the debug functionality is used, this may be necessary.
I add the debug package to comply with the current client so we keep this package this functionality should be here.
But for compression stuff, I am not sure if should we include it or not. Please see conversation; #1444 (comment)

shortcuts · 2023-04-05T13:04:29Z

clients/algoliasearch-client-go/algolia/internal/errs/no_more_host_to_try_err.go

We might see more and more custom error, it might be worth to align on some names in out contributing documentation so that other clients can benefit it

doesn't have to be tackled in this PR, but worth adding a ticket to tackle it

Yes, I will open up a ticket in general about error handling to make it more standard.

shortcuts · 2023-04-05T13:05:49Z

clients/algoliasearch-client-go/algolia/internal/transport/requester.go

+	"time"
+)
+
+const (


just wondering but are they the same as the current client?

yeap, they are the same as the current client, ref: https://github.com/algolia/algoliasearch-client-go/blob/master/algolia/transport/requester.go#L9-L14

shortcuts · 2023-04-05T13:15:34Z

just for context about the default values, they are set at the engine level so we don't need them in the API clients, that's why they are only in the specs

millotp

looks good ! nothing more to add except the localVar

shortcuts

Gg!!

mehmetaligok added 2 commits March 31, 2023 10:16

feat(go): template improvements

606dd07

feat(go): add transport layer to fulfill client requirements

d09caa9

mehmetaligok added enhancement New feature or request Go labels Apr 4, 2023

mehmetaligok self-assigned this Apr 4, 2023

fix(go): remove debug timeout

527cb0f

mehmetaligok marked this pull request as ready for review April 4, 2023 11:49

mehmetaligok requested a review from a team as a code owner April 4, 2023 11:49

mehmetaligok requested review from aallam and shortcuts April 4, 2023 11:49

shortcuts reviewed Apr 4, 2023

View reviewed changes

millotp reviewed Apr 4, 2023

View reviewed changes

feat(go): small review comments

ca354a6

mehmetaligok requested review from millotp and shortcuts April 4, 2023 14:35

feat(go): remove default usage from specs

58349a0

Merge branch 'main' into feat/go-mustache-template-improvements

02a83a7

shortcuts previously approved these changes Apr 5, 2023

View reviewed changes

millotp reviewed Apr 5, 2023

View reviewed changes

feat(go): remove localVar prefix from api template

feef2aa

mehmetaligok dismissed shortcuts’s stale review via feef2aa April 5, 2023 13:34

mehmetaligok requested a review from millotp April 5, 2023 13:35

mehmetaligok requested a review from shortcuts April 5, 2023 15:47

shortcuts approved these changes Apr 5, 2023

View reviewed changes

mehmetaligok merged commit ca763fb into main Apr 6, 2023

mehmetaligok deleted the feat/go-mustache-template-improvements branch April 6, 2023 07:58

+              	"time"
+              )
+              const (

feat(go): template improvements #1444

feat(go): template improvements #1444

Conversation

mehmetaligok commented Apr 4, 2023

🧭 What and Why

Changes included:

🧪 Test

netlify bot commented Apr 4, 2023 • edited Loading

✅ Deploy Preview for api-clients-automation ready!

algolia-bot commented Apr 4, 2023 • edited Loading

✗ The generated branch has been deleted.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millotp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millotp commented Apr 4, 2023 • edited Loading

mehmetaligok commented Apr 4, 2023

millotp commented Apr 4, 2023

mehmetaligok commented Apr 4, 2023

mehmetaligok commented Apr 4, 2023 • edited Loading

shortcuts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shortcuts commented Apr 5, 2023

millotp left a comment

Choose a reason for hiding this comment

shortcuts left a comment

Choose a reason for hiding this comment

netlify bot commented Apr 4, 2023 •

edited

Loading

algolia-bot commented Apr 4, 2023 •

edited

Loading

millotp commented Apr 4, 2023 •

edited

Loading

mehmetaligok commented Apr 4, 2023 •

edited

Loading