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

Fix go scalar collection cast #4388

Merged
merged 3 commits into from
Mar 25, 2024
Merged

Conversation

lampajr
Copy link
Contributor

@lampajr lampajr commented Mar 22, 2024

Fixes #4380

Given openapi:

    post:
      description: 'Create post'
      requestBody:
        required: true
        content:
          application/json:
            schema:
              type: array
              items:
                format: int32
                type: integer

The expected ToPostRequestInformation would be:

// ToPostRequestInformation create post
// returns a *RequestInformation when successful
func (m *PostsRequestBuilder) ToPostRequestInformation(ctx context.Context, body []int32, requestConfiguration *PostsRequestBuilderPostRequestConfiguration) (*i2ae4187f7daee263371cb1c977df639813ab50ffa529013b7437480d1ec0158f.RequestInformation, error) {
	requestInfo := i2ae4187f7daee263371cb1c977df639813ab50ffa529013b7437480d1ec0158f.NewRequestInformationWithMethodAndUrlTemplateAndPathParameters(i2ae4187f7daee263371cb1c977df639813ab50ffa529013b7437480d1ec0158f.POST, "{+baseurl}/posts", m.BaseRequestBuilder.PathParameters)
	if requestConfiguration != nil {
		requestInfo.Headers.AddAll(requestConfiguration.Headers)
		requestInfo.AddRequestOptions(requestConfiguration.Options)
	}
	requestInfo.Headers.TryAdd("Accept", "application/json")
	cast := make([]interface{}, len(body))
	for i, v := range body {
		cast[i] = v
	}
	requestInfo.SetContentFromScalarCollection(ctx, m.BaseRequestBuilder.RequestAdapter, "application/json", cast)
	return requestInfo, nil
}

Note the array cast:

	cast := make([]interface{}, len(body))
	for i, v := range body {
		cast[i] = v
	}

Which is requried as you cannot pass []int32 as []interface{} without this trick.

@lampajr
Copy link
Contributor Author

lampajr commented Mar 22, 2024

@microsoft-github-policy-service agree

@lampajr
Copy link
Contributor Author

lampajr commented Mar 22, 2024

I did not create a new test as there was already existing one checking that specific scenarion: WritesRequestGeneratorBodyForScalarCollection, the issue is that it was not expecting that cast at all.

@lampajr
Copy link
Contributor Author

lampajr commented Mar 22, 2024

Hi @baywet , here my proposal to fix the linked issue.

I was not able to try that locally as I am facing issues while building the project following the steps here, I am getting:

MSBuild version 17.8.5+b5265ef37 for .NET
  Determining projects to restore...
  All projects are up-to-date for restore.
  KiotaGenerated -> /home/alampare/repos/kiota/src/Kiota.Generated/bin/Debug/netstandard2.0/KiotaGenerated.dll
CSC : error CS9057: The analyzer assembly '/home/alampare/repos/kiota/src/Kiota.Generated/bin/Debug/netstandard2.0/KiotaGenerated.dll' references version '4.9.0.0' of the compiler, which is newer than the currently running version '4.8.0.0'. [/home/alampare/repos/kiota/src/Kiota.Builder/Kiota.Builder.csproj]

Here you are suggesting to update to the latest SDK but I am already on that one, any other hint? Sorry but I am not .NET expert 🙏

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Great work! thanks for putting this together!
About dotnet: what does dotnet --version and dotnet --list-sdks return? what OS are you running things on?
Can you also add a entry to the changelog please?

@lampajr
Copy link
Contributor Author

lampajr commented Mar 22, 2024

About dotnet: what does dotnet --version and dotnet --list-sdks return? what OS are you running things on?

Here all infos:

cat /etc/fedora-release
Fedora release 38 (Thirty Eight)

dotnet --version
8.0.102

dotnet --list-sdks
8.0.102 [/usr/lib64/dotnet/sdk]

Can you also add a entry to the changelog please?

Updated 7089517

@baywet
Copy link
Member

baywet commented Mar 22, 2024

latest is 203, but because you're on linux, you might be getting only the 100 patch branch for some reason.
any chance you could use a Codespace for the purpose of testing?

@lampajr
Copy link
Contributor Author

lampajr commented Mar 22, 2024

latest is 203, but because you're on linux, you might be getting only the 100 patch branch for some reason. any chance you could use a Codespace for the purpose of testing?

TBH I never used Codespace, I just tried but it looks like it automatically setups a dev container with .NET 7 instead of .NET 8, maybe there are some configuration in the repo that are forcing that dev container setup (?)

[edit] I also tried to use Fedora 39 locally but the latest sdk version is still the same 8.0.102 [/usr/lib64/dotnet/sdk]

@baywet
Copy link
Member

baywet commented Mar 22, 2024

My bad, I forgot to upgrade the dev container configuration when we moved to net 8.
I just put together #4391 for future use.
Now, the CI is failing, which I expected since you couldn't run unit tests locally.
What I suggest you to do here it to:

  1. create a temporary branch of this one in your repo
  2. merge my changes from 4391 in that temp branch

This way you should be able to run/debug unit tests (I just tried in my own container instance).

Once you have fixes, you can cherry pick the changes from that temp branch onto the current branch.

(or if it's too much gitfu, you can wait for my other PR to get merged, and rebase your branch)

@lampajr
Copy link
Contributor Author

lampajr commented Mar 23, 2024

Thanks @baywet !

I was able to run test using ther devcontainer approach and now they should be fixed ✔️

Is there a way to build the CLI from this branch (download it) and run against my example ? Just to be 100% sure the change works as expected and no regressions are introduced.

@lampajr lampajr force-pushed the gh4380_go_scalar_array branch from bd1a795 to da0febc Compare March 25, 2024 08:55
@lampajr lampajr force-pushed the gh4380_go_scalar_array branch from da0febc to b0337cf Compare March 25, 2024 09:02
@lampajr lampajr marked this pull request as ready for review March 25, 2024 09:02
@lampajr lampajr requested a review from a team as a code owner March 25, 2024 09:02
@lampajr lampajr requested a review from baywet March 25, 2024 09:02
@lampajr
Copy link
Contributor Author

lampajr commented Mar 25, 2024

With commit b0337cf I fixed the broken test and I also added some additional checks on other test cases to check whether the cast is generated or not.

[edit] I also rebased on top of main

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thanks for the continuous work on this!

Is there a way to build the CLI from this branch (download it) and run against my example ? Just to be 100% sure the change works as expected and no regressions are introduced.

yes, you can directly run dotnet run -c Release --project src/kiota/kiota.csproj -- <arguments you would pass> to test things out.
You can alternatively edit one of the launch.json profiles (make sure you don't commit it)
I'll hold merging until you give me a confirmation

@lampajr
Copy link
Contributor Author

lampajr commented Mar 25, 2024

yes, you can directly run dotnet run -c Release --project src/kiota/kiota.csproj -- <arguments you would pass> to test things out.

Thanks, it worked! Maybe that's something that could be added in CONTRIBUTING.md as this could be a very useful information, especially for those who are not familiar with dotnet ecosystem.

I'll hold merging until you give me a confirmation

I was able to run it against my use cases and it works fine! @baywet green light fmpov 🚀

@baywet baywet merged commit 95091be into microsoft:main Mar 25, 2024
199 checks passed
@baywet
Copy link
Member

baywet commented Mar 25, 2024

Thanks for the contribution!
Please go ahead a open another pr to the contributing file for the guidance we discussed about if you feel it could be valuable to others.

@lampajr lampajr deleted the gh4380_go_scalar_array branch March 26, 2024 10:31
@lampajr lampajr mentioned this pull request Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Go: Broken Generated Code when Body is an Array of Scalar Values
2 participants