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 for BaseUrl in C# base class not being used #4880

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

soligen2010
Copy link

@soligen2010 soligen2010 commented May 3, 2024

Fix to use the BaseUrl from the base class when there is a base class and GenerateBaseUrlProperty is not set.

See issue #4764

Edit: This is for C# code generation

Fix to use the BaseUrl from the base class when there is a base class and GenerateBaseUrlProperty is not set.
@soligen2010 soligen2010 changed the title Fix for BaseUrl in base class not being used Fix for BaseUrl in C# base class not being used May 3, 2024
Fixed If in template and added test case for when there is a base class
@soligen2010
Copy link
Author

Added new commit to fix the If statement and added a new test case for when there is a BaseClass.

@soligen2010
Copy link
Author

Can someone please help me understand why this PR is failing? Looking at the errors in the latest run it doesn't seem to me to be related to something I changed. Is there something more I need to do?

Removed the 4 extra spaces that caused the generated file to vary from the  verified file
@soligen2010
Copy link
Author

I finally got my local installation working properly and found that I had 4 extra spaces added to a line in the generated client which caused the file to not match the verified. Hopefully this time the workflow works.

@soligen2010
Copy link
Author

Everything runs successfully on my local machine. The workflow error is

"You must install or update .NET to run this application."

I don't understand how anything I did could cause this, and I can't do anything about what is installed on the workflow server.

Can someone please advise?

Thanks

@Numpsy
Copy link
Contributor

Numpsy commented May 7, 2024

"You must install or update .NET to run this application."

Looks like it's trying to run the tests as .NET 6.0, and the build agent only has 7.0/8.0 installed.

Possibly because the CI build is set to use macos-latest and that recently became macOS 14 running on ARM cpus? (so nothing specific to do with the change here)

@franklixuefei
Copy link

does this PR have any recent tractions?

@soligen2010
Copy link
Author

I don't know what you mean by "recent tractions" This PR addresses an issue that was first introduced in 14.0.1, possibly introduced by PR #4691. There were further changes in this area after that PR, but they didn't fix this issue.

I believe this PR fixes issues #4764 and #4705.

This PR is needed by anyone setting BaseUrl in a base class. For anyone who does this, the recent versions are not usable - they cant upgrade to the most recent version.

@akeijzer11
Copy link

When will this issue be resolved?
Since 14.0.1 i tried every new release if the issue was fixed but setting the BaseUrl in a base class is still not possible.

@soligen2010
Copy link
Author

@RicoSuter Can you please provide some feedback on acceptance of this pull request. This issue is affecting a number of people who are waiting for it. I know the MacOS checks failed, but this appears to be an issue with the build server, which I can't fix, and I don't see a way I can re-run the checks to see if the problem has been resolved.

Thanks

@Numpsy
Copy link
Contributor

Numpsy commented Jun 13, 2024

I don't see a way I can re-run the checks to see if the problem has been resolved.

Maybe update or rebase your branch on top of the latest master and push that to trigger the CI again?

@@ -290,7 +292,11 @@
{% endif -%}

var urlBuilder_ = new System.Text.StringBuilder();
{% if UseBaseUrl %}if (!string.IsNullOrEmpty(_baseUrl)) urlBuilder_.Append(_baseUrl);{% endif %}
{% if HasBaseClass == true and GenerateBaseUrlProperty == false -%}
Copy link
Owner

Choose a reason for hiding this comment

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

Is this correct? Not using UseBaseUrl anymore...

@RicoSuter
Copy link
Owner

You know that you can overwrite templates on your side without changing NSwag itself?

I'm super careful changing these templates because in the past PRs like this broke a lot of stuff and I had to work days to fix all issues introduced by untested PRs...

@soligen2010
Copy link
Author

soligen2010 commented Jul 1, 2024

@RicoSuter thanks for coming into the discussion. I am willing to help with anything I can do to resolve this issue.

I think the issue is that Base Url has been removed even when using a base class to set the Base Url, and there are a number of us (There are a couple of issue threads out there concerning this) that have base classes that do this. In the template, I have set the IF conditions so that behavior is only changed if a Base Class is used and GenerateBaseUrlProperty is false. In these cases the Base URL previously was controlled by the Base Class, and I think all of us affected would agree that we would like this capability restored.

If there is another way we should be now be doing this same thing, then please let me know and I will be happy to try it. I really would prefer to not maintain a fork with customized templates as I think that would cause issues in my organization for version upgrades.

To my mind, this change simply restores the ability to control the Base Url in the base class. If this pull request isn’t acceptable, how else should we change the base URL dynamically? If this capability is intentionally permanently removed, what is your recommendation on how we should do this?

For background, I just changed from Swagger/AutoRest to all NSwag in January. I implemented using a base class because then I could have a similar constructor that minimizes the code remediation in the projects that used the old AutoRest clients (BaseUrl came in on the constructor in our AutoRest clients). About a month after changing to NSwag, this stopped working (It partially broke in 14.0.2 (I found a work around). It fully broke in 14.0.3. Maybe there is a alternate way to do things, but it would likely be a lot of work to re-structure the existing code base to work differently.

Please let me know how to proceed. I am committed to helping resolve this issue.

@cculver
Copy link

cculver commented Jul 1, 2024

Thanks @soligen2010, your comment echoes my use case and I am eagerly awaiting a solution to this problem. I am also happy to adjust if there is a way.

@RicoSuter
Copy link
Owner

This whole BaseUrl is now super messed up and very hard to understand... My personal recommendation is to not use BaseURL at all and instead set the base URL on the injected HttpClient... But will still look into fixing this.

@soligen2010
Copy link
Author

This whole BaseUrl is now super messed up and very hard to understand... My personal recommendation is to not use BaseURL at all and instead set the base URL on the injected HttpClient... But will still look into fixing this.

That was one of the first things I tried, but doesn't work with our code base - it probably should work if things are done "right", but for me right now, that would involve a lot of remediation in the code base. Looking through things again, perhaps it can be set using PrepareRequest. I'll give it a try and report back.

@soligen2010
Copy link
Author

soligen2010 commented Jul 2, 2024

Looking at this again, I agree there are issues with the base url logic. In the current release (14.0.8) when a Base Class is used and GenerateBaseUrlProperty is false, this following is generated.

#pragma warning disable 8618
private string _baseUrl;
#pragma warning restore 8618

And it is used in each endpoint like this.

if (!string.IsNullOrEmpty(_baseUrl)) urlBuilder_.Append(_baseUrl);

However, there is no way to actually set a value for _baseUrl, so these lines of code never do anything useful. This pull request removes _baseUrl, and instead uses BaseUrl from the base class instead so it seems to me it wont break other use cases.

As for using PrepareRequest, That is looking viable as a work around, but I would prefer the BaseUrl solution. I will post more on that after some testing.

@soligen2010
Copy link
Author

The PrepareRequest work around worked. I prefer the BaseUrl solution because PrepareRequest is a partial method, so this code would need to be copied into every client. Having it all in BaseClass allows me to distribute the solution via our internal NuGet server.

Another alternative would be to add a virtual method like PrepareRequest in the template so that the BaseClass can override it.

Below is the workaround I came up with using PrepareRequest. It must be in a file using the same namespace as the generated client class. @RicoSuter please comment on how you would like to proceed in resolving this issue.

    public partial class MyApi : MyNamespace.MyClientBase, IMyApi
    {
        partial void PrepareRequest(System.Net.Http.HttpClient client, System.Net.Http.HttpRequestMessage request, System.Text.StringBuilder urlBuilder)
        {
            if (!string.IsNullOrWhiteSpace(this.BaseUrl) && client?.BaseAddress is null)
            {
                var testUri = new Uri(urlBuilder.ToString(), UriKind.RelativeOrAbsolute);
                if (!testUri.IsAbsoluteUri)
                {
                    urlBuilder.Insert(0, this.BaseUrl);
                }
            }
        }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants