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.

Sign up for GitHub

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

Skip empty clients #5652

Closed
wants to merge 2 commits into from
Closed

Skip empty clients #5652

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
806629f
Skip empty clients
live1206 Jan 17, 2025
fcaa37b
refine logic and add tests
live1206 Jan 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...t-csharp/generator/Microsoft.Generator.CSharp.ClientModel/src/Providers/ClientProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ private record OAuth2Fields(FieldProvider AuthField, FieldProvider Authorization
private Lazy<IReadOnlyList<FieldProvider>> _additionalClientFields;

private Lazy<ParameterProvider?> ClientOptionsParameter { get; }
internal IReadOnlyList<ClientProvider> SubClients => _subClients.Value;

// for mocking
#pragma warning disable CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider declaring as nullable.
Expand Down
29 changes: 27 additions & 2 deletions ...http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel/src/ScmTypeFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,33 @@ private ExtensibleEnumSerializationProvider[] CreateExtensibleEnumSerializations
}
}

ClientCache[inputClient] = client;
return client;
var result = client is not null && IsValidClient(inputClient, client) ? client : null;
ClientCache[inputClient] = result;
return result;
}

private bool IsValidClient(InputClient inputClient, ClientProvider client)
{
// client is not valid if it has no operations, sub-client methods, or custom code methods
if (inputClient.Operations.Count > 0)
{
return true;
}

foreach (var subclient in client.SubClients)
{
if (subclient.Methods.Count > 0)
Copy link
Contributor

@JoshLove-msft JoshLove-msft Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Checking for the Methods for the subclient isn't enough - we either need to check for the operations on the input client or the methods on the client and its REST client. I think adding an internal property like HasOperations or HasMethods to ClientProvider should be added to encapsulate this logic.

Copy link
Contributor Author

@live1206 live1206 Jan 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

My initial thought is that, we should have the same criteria for client, sub-client and custom code, which is any of them should have some methods.

For clientProvider, if there is no methods which means there is no operation or helper methods. Given RestClientProvider only contains methods of CreateRequestMethod, we should not need to check RestClientProvider.

Same thing for subClient, which is also a clientProvider.

For custom code, I don't understand why user would like to have a custom property or field to hold the empty client.
Do you have a use case for this?

{
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to also check to see if the subclients have custom code defined.

Copy link
Contributor Author

@live1206 live1206 Jan 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

added the check of custom code but only check methods of custom code for now.

}
}

if (client.CustomCodeView?.Methods.Count > 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Checking for the Methods isn't enough because the custom code may be using Properties, Fields, or even just doing something in the constructor. We should simply check whether there is custom code or not, and not worry about the individual members.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, it seems like custom code has a different criteria that if there is any custom code exists we should not skip the client?
Because for client and sub-client, we only check the methods.

{
return true;
}

return false;
}

protected virtual ClientProvider? CreateClientCore(InputClient inputClient) => new ClientProvider(inputClient);
Expand Down
2 changes: 1 addition & 1 deletion ...tor.CSharp.ClientModel/test/Providers/ClientProviders/ClientProviderCustomizationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ public async Task CanRenameSubClient()
InputFactory.Parameter("p1", InputFactory.Array(InputPrimitiveType.String))
]);
var inputClient = InputFactory.Client("TestClient", operations: [inputOperation]);
InputClient subClient = InputFactory.Client("custom", [], [], inputClient.Name);
InputClient subClient = InputFactory.Client("custom", [inputOperation], [], inputClient.Name);
var plugin = await MockHelpers.LoadMockPluginAsync(
clients: () => [inputClient, subClient],
compilation: async () => await Helpers.GetCompilationFromDirectoryAsync());
Expand Down
14 changes: 9 additions & 5 deletions ...nerator.CSharp.ClientModel/test/Providers/ClientProviders/ClientProviderSubClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,15 @@ namespace Microsoft.Generator.CSharp.ClientModel.Tests.Providers.ClientProviders
public class ClientProviderSubClientTests
{
private const string TestClientName = "TestClient";
private static readonly InputClient _animalClient = new("animal", string.Empty, "AnimalClient description", [], [], TestClientName);
private static readonly InputClient _dogClient = new("dog", string.Empty, "DogClient description", [], [], _animalClient.Name);
private static readonly InputClient _catClient = new("cat", string.Empty, "CatClient description", [], [], _animalClient.Name);
private static readonly InputClient _hawkClient = new("hawkClient", string.Empty, "HawkClient description", [], [], _animalClient.Name);
private static readonly InputClient _huskyClient = new("husky", string.Empty, "HuskyClient description", [], [], _dogClient.Name);
private static readonly InputOperation _inputOperation = InputFactory.Operation("HelloAgain", parameters:
[
InputFactory.Parameter("p1", InputFactory.Array(InputPrimitiveType.String))
]);
private static readonly InputClient _animalClient = new("animal", string.Empty, "AnimalClient description", [_inputOperation], [], TestClientName);
private static readonly InputClient _dogClient = new("dog", string.Empty, "DogClient description", [_inputOperation], [], _animalClient.Name);
private static readonly InputClient _catClient = new("cat", string.Empty, "CatClient description", [_inputOperation], [], _animalClient.Name);
private static readonly InputClient _hawkClient = new("hawkClient", string.Empty, "HawkClient description", [_inputOperation], [], _animalClient.Name);
private static readonly InputClient _huskyClient = new("husky", string.Empty, "HuskyClient description", [_inputOperation], [], _dogClient.Name);

[SetUp]
public void SetUp()
Expand Down
60 changes: 56 additions & 4 deletions ...rosoft.Generator.CSharp.ClientModel/test/Providers/ClientProviders/ClientProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,13 @@ public class ClientProviderTests
private const string KeyAuthCategory = "WithKeyAuth";
private const string OAuth2Category = "WithOAuth2";
private const string TestClientName = "TestClient";
private static readonly InputClient _animalClient = new("animal", "", "AnimalClient description", [], [], TestClientName);
private static readonly InputClient _dogClient = new("dog", "", "DogClient description", [], [], _animalClient.Name);
private static readonly InputClient _huskyClient = new("husky", "", "HuskyClient description", [], [], _dogClient.Name);
private static readonly InputOperation _inputOperation = InputFactory.Operation("HelloAgain", parameters:
[
InputFactory.Parameter("p1", InputFactory.Array(InputPrimitiveType.String))
]);
private static readonly InputClient _animalClient = new("animal", "", "AnimalClient description", [_inputOperation], [], TestClientName);
private static readonly InputClient _dogClient = new("dog", "", "DogClient description", [_inputOperation], [], _animalClient.Name);
private static readonly InputClient _huskyClient = new("husky", "", "HuskyClient description", [_inputOperation], [], _dogClient.Name);
private static readonly InputModelType _spreadModel = InputFactory.Model(
"spreadModel",
usage: InputModelTypeUsage.Spread,
Expand Down Expand Up @@ -63,6 +67,54 @@ public void SetUp()
clientPipelineApi: TestClientPipelineApi.Instance);
}

[Test]
public void TestEmptyClient()
Copy link
Contributor

@JoshLove-msft JoshLove-msft Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We need a test that validates the behavior when custom code is added for the client and for a sub client. See https://github.com/microsoft/typespec/blob/main/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel/test/Providers/ClientProviders/ClientProviderCustomizationTests.cs for examples of custom code tests. Probably the custom code tests should go in that file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests, but only for custom code with methods for now

{
var client = InputFactory.Client(TestClientName);
var plugin = MockHelpers.LoadMockPlugin(clients: () => [client]);

var clientProvider = plugin.Object.OutputLibrary.TypeProviders.SingleOrDefault(t => t is ClientProvider && t.Name == TestClientName);
var restClientProvider = plugin.Object.OutputLibrary.TypeProviders.SingleOrDefault(t => t is RestClientProvider && t.Name == TestClientName);
Assert.IsNull(clientProvider);
Assert.IsNull(restClientProvider);
}

[Test]
public void TestNonEmptySubClient()
{
var inputOperation = InputFactory.Operation("HelloAgain", parameters:
[
InputFactory.Parameter("p1", InputFactory.Array(InputPrimitiveType.String))
]);
var client = InputFactory.Client(TestClientName);
var subClient = InputFactory.Client($"Sub{TestClientName}", [inputOperation], [], client.Name);
var plugin = MockHelpers.LoadMockPlugin(clients: () => [client, subClient]);

var subClientProvider = plugin.Object.OutputLibrary.TypeProviders.SingleOrDefault(t => t is ClientProvider && t.Name == subClient.Name);
Assert.IsNotNull(subClientProvider);

var clientProvider = plugin.Object.OutputLibrary.TypeProviders.SingleOrDefault(t => t is ClientProvider && t.Name == TestClientName);
Assert.IsNotNull(clientProvider);
}

[Test]
public void TestEmptySubClient()
{
var client = InputFactory.Client(TestClientName);
var subClient = InputFactory.Client($"Sub{TestClientName}", [], [], client.Name);
var plugin = MockHelpers.LoadMockPlugin(clients: () => [client, subClient]);

var subClientProvider = plugin.Object.OutputLibrary.TypeProviders.SingleOrDefault(t => t is ClientProvider && t.Name == subClient.Name);
var subRestClientProvider = plugin.Object.OutputLibrary.TypeProviders.SingleOrDefault(t => t is RestClientProvider && t.Name == subClient.Name);
Assert.IsNull(subClientProvider);
Assert.IsNull(subRestClientProvider);

var clientProvider = plugin.Object.OutputLibrary.TypeProviders.SingleOrDefault(t => t is ClientProvider && t.Name == TestClientName);
var restClientProvider = plugin.Object.OutputLibrary.TypeProviders.SingleOrDefault(t => t is RestClientProvider && t.Name == TestClientName);
Assert.IsNull(clientProvider);
Assert.IsNull(restClientProvider);
}

[Test]
public void TestBuildProperties()
{
Expand Down Expand Up @@ -736,7 +788,7 @@ public void TestApiVersionPathParameterOfClient(InputClient inputClient)
public void ClientProviderIsAddedToLibrary()
{
var plugin = MockHelpers.LoadMockPlugin(
clients: () => [new InputClient("test", "test", "test", [], [], null)]);
clients: () => [new InputClient("test", "test", "test", [_inputOperation], [], null)]);

Assert.AreEqual(1, plugin.Object.OutputLibrary.TypeProviders.OfType<ClientProvider>().Count());
}
Expand Down
7 changes: 6 additions & 1 deletion ...rp.ClientModel/test/Providers/RestClientProviders/RestClientProviderCustomizationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Linq;
using System.Threading.Tasks;
using Microsoft.Generator.CSharp.ClientModel.Providers;
using Microsoft.Generator.CSharp.Input;
using Microsoft.Generator.CSharp.Primitives;
using Microsoft.Generator.CSharp.Tests.Common;
using NUnit.Framework;
Expand All @@ -16,7 +17,11 @@ public class RestClientProviderCustomizationTests
[Test]
public async Task CanChangeClientNamespace()
{
var inputClient = InputFactory.Client("TestClient");
var inputOperation = InputFactory.Operation("HelloAgain", parameters:
[
InputFactory.Parameter("p1", InputFactory.Array(InputPrimitiveType.String))
]);
var inputClient = InputFactory.Client("TestClient", [inputOperation]);
var plugin = await MockHelpers.LoadMockPluginAsync(
clients: () => [inputClient],
compilation: async () => await Helpers.GetCompilationFromDirectoryAsync());
Expand Down
21 changes: 21 additions & 0 deletions ...ClientProviders/TestData/RestClientProviderCustomizationTests/CanChangeClientNamespace.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,31 @@

#nullable disable

using System.ClientModel;
using System.ClientModel.Primitives;
using Sample;

namespace Sample.Custom
{
/// <summary></summary>
public partial class TestClient
{
private static global::System.ClientModel.Primitives.PipelineMessageClassifier _pipelineMessageClassifier200;

private static global::System.ClientModel.Primitives.PipelineMessageClassifier PipelineMessageClassifier200 => _pipelineMessageClassifier200 = global::System.ClientModel.Primitives.PipelineMessageClassifier.Create(stackalloc ushort[] { 200 });

internal global::System.ClientModel.Primitives.PipelineMessage CreateHelloAgainRequest(global::System.ClientModel.BinaryContent content, global::System.ClientModel.Primitives.RequestOptions options)
{
global::System.ClientModel.Primitives.PipelineMessage message = Pipeline.CreateMessage();
message.ResponseClassifier = PipelineMessageClassifier200;
global::System.ClientModel.Primitives.PipelineRequest request = message.Request;
request.Method = "GET";
global::Sample.ClientUriBuilder uri = new global::Sample.ClientUriBuilder();
uri.Reset(_endpoint);
request.Uri = uri.ToUri();
request.Content = content;
message.Apply(options);
return message;
}
}
}
Loading