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

C# code generation with /GenerateBaseUrlProperty:false option is broken in v14.0.1 #4705

Open
olegd-superoffice opened this issue Jan 13, 2024 · 14 comments

Comments

@olegd-superoffice
Copy link
Contributor

NSwag generates invalid C# code when /GenerateBaseUrlProperty:false option is used. It looks like it is a regression in v14.0.1 possibly caused by #4691
Here's a simple project which demonstrates the regression: GenerateBaseUrlPropertyFalse.zip. It will download and use example petstore OpenAPI document. Just run dotnet build.
It works with NSwag.ApiDescription.Client 13.20.0 and 14.0.0 but fails with 14.0.1

@paulomorgado

@paulomorgado
Copy link
Contributor

@olegd-superoffice, please, explain in what way it doesn't work.

@paulomorgado
Copy link
Contributor

@RicoSuter,

I think I'm getting something wrong here.

Is the _baseUrl field supposed to be used anywhere other than inside the BaseUrl property?

HasBaseUrl indicates that the base URL from the API definition is to be used, right?

UseBaseUrl indicates that a base URL should be used, right?

GenerateBaseUrlProperty indicates that a BaseUrl property is to be generated, right?

InjectHttpClient indicates that an HttpClient property is to be injected, right?

The difference between 14.0.1 and 14.0.0 and 13.20.0 is that 14.0.1 assigns the _baseUrl field without declaring it, but all versions use the baseUrl property without declaring it when /GenerateBaseUrlProperty:false is specified.

My reasoning how this should be is:

  • if UseBaseUrl is true, _baseUrl or BaseUrl should be used, depending on BaseUrl being generated or not.
    • Or always use BaseUrl and make it public only if /GenerateBaseUrlProperty:true.
  • if UseBaseUrl is true
    • if HasBaseUrl is true , set either _baseUrl or BaseUrl to the sanitzed BaseUrl.
      • Or always use BaseUrl and make it public only if /GenerateBaseUrlProperty:true.
    • if HasBaseUrl is false , add a baseUrl parameter to the constructor and set either _baseUrl or BaseUrl to the sanitzed BaseUrl.
      • Or always use BaseUrl and make it public only if /GenerateBaseUrlProperty:true.

Is that how it should be?

@paulomorgado
Copy link
Contributor

@RicoSuter,

Seems like the change is to never initialize the _baseUrlfield and initialize BaseUrl property instead.

And there's a breaking change for implementers of the BaseUrl property that need to provide a / terminated URL.

@W0GER
Copy link

W0GER commented Jan 30, 2024

But why force implementers to provide a /terminated URL. This seems like a potential spot for bugs.

If the BaseUrl has a value, version 13.20 would check to see if it had a trailing / before concatenating with the rest of the URL and query strings.

@paulomorgado
Copy link
Contributor

I changed it as a performance optimization, but I'm rethinking it.

@RicoSuter
Copy link
Owner

Should be fixed with this commit:
be42e58

Sorry for all the inconveniences...

@olegd-superoffice
Copy link
Contributor Author

@RicoSuter The 14.0.3 generated code is getting compiled without errors now but the logic of how
/GenerateBaseUrlProperty:false flag works is different now.

  • Up to (and including) 14.0.0 it meant that BaseUrl property was not generated but still was used and it was responsibility of base class to declare and assign a BaseUrl value (usually from configuration).
  • In 14.0.1 and 14.0.2 it was broken and code generated with /GenerateBaseUrlProperty:false flag didn't compile.
  • In 14.0.3 code is getting compiled again but works differently - BaseUrl property is not generated and is not used at all. Instead private string _baseUrl field is assigned from a host value defined in OpenAPI document. The only workaround I could find to change _baseUrl value is to put some weird logic into override of PrepareRequest method.

It looks like it is still a breaking change between 14.0.0 and 14.0.3.

@paulomorgado
Copy link
Contributor

I think I was addressing all issues with paulomorgado@744940b, but it's now failing this recently added test:

[Fact]
public async Task WhenUsingBaseUrl_ButNoProperty_ThenPropertyIsNotUsedAndFieldIsGenerated()
{
    // Arrange
    var swaggerGenerator = new WebApiOpenApiDocumentGenerator(new WebApiOpenApiDocumentGeneratorSettings
    {
        SchemaSettings = new NewtonsoftJsonSchemaGeneratorSettings()
    });
    var document = await swaggerGenerator.GenerateForControllerAsync<FooController>();
    var generator = new CSharpClientGenerator(document, new CSharpClientGeneratorSettings
    {
        UseBaseUrl = true,
        GenerateBaseUrlProperty = false
    });
    // Act
    var code = generator.GenerateFile();
    // Assert
    Assert.DoesNotContain("BaseUrl", code);
    Assert.Contains("string _baseUrl", code);
}

I still don't have a full grasp of the usage of -baseUrl and BaseUrl.

@alexcheveau
Copy link

I can confirm the impact on my project as well.

My use case is:
useBaseUrl=true
generateBaseUrlProperty=false

I have a base class that implements BaseURL and return the urlto be used in urlBuilder but now it's ignored.
The proxy is using directly _baseUrl instead of BaseURL

@Trapulo
Copy link

Trapulo commented Feb 15, 2024

I also have this problem. I cannot assign baseUrl in base class because generated classes use locally defined _baseUrl ignoring external settings.

@soligen2010
Copy link

For me, 14.0.1 works as expected using BaseUrl from the base class
14.0.2 works as long as the swagger.json does not have a BasePath. If it has a BasePath I get a compile error because _baseUrl is not defined.
14.0.3 compiles, but the BasePath in the base class gets ignored, so things don't work.

I am running with UseBaseUrl = true and GenerateBaseUrlProperty = false.

For my application, I need to be able to set the base Url at run time, even if a Base Path is specified in swagger.json. Is this going to be changed back so the base class can control the base Url, or do I need to find another way to do this?

For now I have to stay on 14.0.1

Thanks

@franklixuefei
Copy link

franklixuefei commented Apr 10, 2024

14.0.7 still doesn't work. It is complaining and the BaseUrl property defined in the base class is now not referenced at all. What's the new logic of setting the base url?
image
image

@patophasselt
Copy link

When will this issue be fixed? The code should use the BaseUrl from the ClientBase instead of hardcoding the original URL in the _baseUrl. Please also revert the logic to trim a trailing '/'. As mentioned above, this potentially introduces bugs.

@dreamofmaks
Copy link

Any updates on the issue?

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

No branches or pull requests

10 participants