🐛 Fix link followers generation to avoid duplicate query params

[aorabdel]

Description

Since we patch the link follower functions that are generated by OpenAPI Generator by changing the behaviour from following a hard-coded link to following a link argument, there was a recent patch by the generator that added default param values to limit, offset and embed if it was missing in the request, but the way we follow link, we follow already constructed links already with the params set up.

Since we already have a link with the correct params, we don't need to add these params to the request, and the generator now appends default value params, which results in a duplication of these params.

Since the behaviour is patched by us to follow generic links, we have to patch this behavior as well.

This patch adds these statements at the beginning of follower functions

	linkHasOffsetParam := false
	linkHasLimitParam := false
	linkHasEmbedParam := false
	if parsedLink, err := url.Parse(link); err == nil {
		linkQuery := parsedLink.Query()
		linkHasOffsetParam = linkQuery.Has("offset")
		linkHasLimitParam = linkQuery.Has("limit")
		linkHasEmbedParam = linkQuery.Has("embed")
	}

Then our generator searches for param checks

if r.limit != nil
// or
if r.offset != nil
// or
if r.embed != nil

and wraps these blocks with our injected guard:

if !linkHasLimitParam
    if r.limit != nil
// or
if !linkHasOffsetParam
    if r.offset != nil
// or
if !linkHasEmbedParam
    if r.embed != nil

Test Coverage

• This change is covered by existing or additional automated tests.
• Manual testing has been performed (and evidence provided) as automated testing was not feasible.
• Additional tests are not required for this change (e.g. documentation update).

[joshjennings98]

What is the reasoning for using the AST for code generation itself?

In the generators we tend to use it for parsing the go code generated by the other tools, but for the generation, I think that templating the go code makes everything a lot more readable. You would avoid having to have a comment explaining what is happening (I always think that a comment for what indicates that the code is too complex and runs the risk of being unmaintainable).

If there is a good reason for generating the go code with the AST stuff, then we can do it, but it would need to be a good reason and I currently can't see a reason for this over doing a parsing run and then generating code based on templates.

[joshjennings98]

Can we have a discussion about what I mentioned in the comments. The code that is generated is good, but I am not sure we should do it via the AST library

[aorabdel]

The main reason I chose modifying the AST, is that this is modifying generated code that changes over time, this generator modifies code generated by the OpenAPI generator, although not perfect, this seems to me to be the most robust way of modifying code that changes over time.

[joshjennings98]

The main reason I chose modifying the AST, is that this is modifying generated code that changes over time, this generator modifies code generated by the OpenAPI generator, although not perfect, this seems to me to be the most robust way of modifying code that changes over time.

I see that for modifying the existing functions this makes a bit more sense, but I don't think this is more resilient as it highly dependent on the code as it exists now (there is nothing stopping them from checking nilness some other way or by adding their own variables that conflict with the ones you added.

However, I see the advantage vs just templates for this use case. But I think we need to do one of two things:

1. Make this a lot more readable since the AST code is hard to follow
2. Take a hybrid approach and use templates for the if statements etc., parse the AST for the bits you need and render them into the template and inject it back. It would require a small amount of boiler plate but it be easier to read. If the code changes it is clear to see based on the template how it will be affected

It is up to you since this is your change, but if you do stick with the full AST approach we need to make sure it is maintainable