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

The GetBindingForEndpoint and GetEndpointAddress generated methods should be public instead of private #5179

Open
0xced opened this issue Jun 22, 2023 · 4 comments · May be fixed by #5180
Open

Comments

@0xced
Copy link
Contributor

0xced commented Jun 22, 2023

This is not a bug per se but rather an annoying behaviour of the svcutil tool.

The svcutil tool generates static methods to get Binding and EndpointAddress for a given client (GetBindingForEndpoint + GetEndpointAddress + optionally GetDefaultBinding and GetDefaultEndpointAddress). Unfortunately all those methods are private and as such can't be easily consumed. Here's an example of what is currently generated.

private static System.ServiceModel.Channels.Binding GetBindingForEndpoint(EndpointConfiguration endpointConfiguration)
{
    if ((endpointConfiguration == EndpointConfiguration.HelloEndpointPort))
    {
        System.ServiceModel.BasicHttpBinding result = new System.ServiceModel.BasicHttpBinding();
        result.MaxBufferSize = int.MaxValue;
        result.ReaderQuotas = System.Xml.XmlDictionaryReaderQuotas.Max;
        result.MaxReceivedMessageSize = int.MaxValue;
        result.AllowCookies = true;
        return result;
    }
    throw new System.InvalidOperationException(string.Format("Could not find endpoint with name \'{0}\'.", endpointConfiguration));
}

private static System.ServiceModel.EndpointAddress GetEndpointAddress(EndpointConfiguration endpointConfiguration)
{
    if ((endpointConfiguration == EndpointConfiguration.HelloEndpointPort))
    {
        return new System.ServiceModel.EndpointAddress("http://apps.learnwebservices.com/services/hello");
    }
    throw new System.InvalidOperationException(string.Format("Could not find endpoint with name \'{0}\'.", endpointConfiguration));
}

private static System.ServiceModel.Channels.Binding GetDefaultBinding()
{
    return HelloEndpointClient.GetBindingForEndpoint(EndpointConfiguration.HelloEndpointPort);
}

private static System.ServiceModel.EndpointAddress GetDefaultEndpointAddress()
{
    return HelloEndpointClient.GetEndpointAddress(EndpointConfiguration.HelloEndpointPort);
}

public enum EndpointConfiguration
{
    HelloEndpointPort,
}

Let's say you want to tweak the default binding to change some timeouts (and possible some other settings). This is currently impossible to do because the generated methods are private.

var binding = HelloEndpointClient.GetDefaultBinding();
binding.OpenTimeout = TimeSpan.FromSeconds(5);
binding.CloseTimeout = TimeSpan.FromSeconds(6);
binding.SendTimeout = TimeSpan.FromSeconds(7);
binding.ReceiveTimeout = TimeSpan.FromSeconds(8);
await using var client = new HelloEndpointClient(binding, HelloEndpointClient.GetDefaultEndpointAddress());

The contents of those generated methods must be duplicated when it needs to be tweaked.

Making those static methods public instead private would solve this issue.

@0xced
Copy link
Contributor Author

0xced commented Jun 22, 2023

I have now opened pull request #5180 to address this issue.

@mconnew
Copy link
Member

mconnew commented Jun 29, 2023

We 100% agree that being able to modify the binding is a really useful mechanism. I'd like to propose a different way to achieve it which I think will be more powerful. What do you think about having a partial method which you can provide the implementation for through a partial class that gets called to modify the binding before it's used? It might be easier if I provide what that would look like:

Generated Code HelloServiceClient.cs

partial class HelloServiceClient
{
    private static System.ServiceModel.Channels.Binding GetBindingForEndpoint(EndpointConfiguration endpointConfiguration)
    {
        if ((endpointConfiguration == EndpointConfiguration.HelloEndpointPort))
        {
            System.ServiceModel.BasicHttpBinding result = new System.ServiceModel.BasicHttpBinding();
            result.MaxBufferSize = int.MaxValue;
            result.ReaderQuotas = System.Xml.XmlDictionaryReaderQuotas.Max;
            result.MaxReceivedMessageSize = int.MaxValue;
            result.AllowCookies = true;
            ConfigureBinding(ref result);
            return result;
        }
        throw new System.InvalidOperationException(string.Format("Could not find endpoint with name \'{0}\'.", endpointConfiguration));
    }

    public static partial void ConfigureBinding(ref binding);
}

Partial class that your own in your project HelloServiceClientCustomizations.cs

partial class HelloServiceClient
{
    public static void ConfigureBinding(ref binding)
    {
        binding.OpenTimeout = TimeSpan.FromSeconds(5);
    }
}

The advantage of this approach is the code is more concise at the point of consumption. If you had 10 places in your code when you new up an instance, the approach of exposing the methods as public would require modify each call point. The approach above would cause it to be automatically applied every time. I don't see a problem with also making these things public, but I think the approach I just outlined should be the primary mechanism for customizing the binding so you don't pollute the point of consumption with knowledge of modifying the binding.

@mconnew
Copy link
Member

mconnew commented Aug 10, 2023

I've given this a bit more thought and you don't need to make it public to use it. You can do this instead:

partial internal class HelloServiceClient
{
    public static Binding GetGeneratedBinding()
    {
        // Because we're a member of HelloServiceClient, we can access all private members
        return GetDefaultBinding();
    }

    public static System.ServiceModel.EndpointAddress GetGeneratedEndpointAddress()
    {
        return GetDefaultEndpointAddress();
    }
}

Then your application code would look like this:

var binding = HelloServiceClient.GetGeneratedBinding();
binding.OpenTimeout = TimeSpan.FromSeconds(5);
binding.CloseTimeout = TimeSpan.FromSeconds(6);
binding.SendTimeout = TimeSpan.FromSeconds(7);
binding.ReceiveTimeout = TimeSpan.FromSeconds(8);
await using var client = new HelloServiceClient(binding, HelloEndpointClient.GetGeneratedEndpointAddress());

@0xced
Copy link
Contributor Author

0xced commented Aug 30, 2023

That's a good point. My claim about it being impossible to do because the generated methods are private was wrong! I see two downsides to this approach, though.

  1. You have to manually create a partial class for each client generated with the svcutil tool.
  2. You can't use GetDefaultBinding and GetDefaultEndpointAddress method names, you have to choose something else. Granted, they could be exposed as DefaultBinding and DefaultEndpointAddress properties, which is prettier than GetGeneratedBinding and GetGeneratedEndpointAddress in my humble opinion.
public partial class HelloEndpointClient
{
    public static Binding DefaultBinding { get; } = GetDefaultBinding();

    public static EndpointAddress DefaultEndpointAddress { get; } = GetDefaultEndpointAddress();
}

Looking at it, I still think it should not be necessary to write such code and that those methods should be exposed publicly.

0xced added a commit to 0xced/Wcf.HttpClientFactory that referenced this issue Aug 30, 2023
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 a pull request may close this issue.

2 participants