Skip to content

Commit

Permalink
Resolve recursive language definations causing incorrect x-ms-pageabl…
Browse files Browse the repository at this point in the history
…e methods (#1303)

# Description
This is a second attempt at #1281, with a bit of a deeper dig into how we ran into the situtation.

Pavel pointed out during a discussion that we really shouldn't be seeing any of these "extra" methods at the level #1281 was filtering them out. 

After applying industrial strength bisect I was able to come up with this test case:  
```yaml
!CodeModel 
operationGroups:
  - !OperationGroup 
    operations:
      - !Operation 
        language: !Languages 
          default:
            paging:
              nextLinkName: nextLink
      - !Operation 
        language: !Languages 
          default:
            paging:
              nextLinkName: nextLink
              nextLinkOperation: !Operation &ref_17
                language: !Languages 
                  default:
                    paging:
                      nextLinkName: nextLink
                      nextLinkOperation: *ref_17
      - *ref_17

```

In it this code:

```csharp
            foreach (var op in v.OperationGroups.First().Operations)
            {
                var paging = op.Language.Default.Paging;
                bool hasNextOperation = paging?.NextLinkOperation != null;
```
would return true for the second and third operation in my simplified model but the third (*ref_17) would differ in the "full" autorest.csharp model.

The culprit was surprisingly:

```
-    internal partial class Language : IDictionary<string, object>
+    internal partial class Language
```

For some, honestly unknown, reason YamlDotNet would not handle this case correctly as long as we implemented the dictionary interface. I decided against deep diving into that 3rd party dependency to understand this further. This already cost too many days. I did try bumping to latest, which failed for unrelated reasons but did not change this behavior.

Removing the IDictionary interface required adding a few fields (header, summary) that were being indirectly handled, and may require more when I test this with azure-sdk-for-net this afternoon.

Test case included for #1056 in particular.

The removals in the paging test suites are expected.
  • Loading branch information
chamons authored Jun 9, 2021
1 parent bfeff3b commit ace7084
Show file tree
Hide file tree
Showing 6 changed files with 29 additions and 306 deletions.
62 changes: 5 additions & 57 deletions src/AutoRest.CSharp/Common/Input/CodeModelPartials.cs
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ internal partial class Paging
}

/// <summary>language metadata specific to schema instances</summary>
internal partial class Language : IDictionary<string, object>
internal partial class Language
{
/// <summary>namespace of the implementation of this item</summary>
[YamlMember(Alias = "namespace")]
Expand All @@ -198,63 +198,11 @@ internal partial class Language : IDictionary<string, object>
[YamlMember(Alias = "paging")]
public Paging? Paging { get; set; }

[YamlIgnore]
public IDictionary<string, object> AdditionalProperties = new Dictionary<string, object>();
[YamlMember(Alias = "header")]
public string? Header { get; set; }

private readonly Dictionary<string, object> _dictionary = new Dictionary<string, object>();
private static readonly Dictionary<string, PropertyInfo> DeserializableProperties = typeof(Language).GetDeserializableProperties();

// Workaround for mapping properties from the dictionary entries
private void AddAndMap(string key, object value)
{
_dictionary.Add(key, value);

if (DeserializableProperties.ContainsKey(key))
{
var propInfo = DeserializableProperties[key];
propInfo.SetValue(this, propInfo.DeserializeDictionary(value));
return;
}

AdditionalProperties.Add(key, value);
}

public IEnumerator<KeyValuePair<string, object>> GetEnumerator() => _dictionary.GetEnumerator();
IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();

public void Add(KeyValuePair<string, object> item) => AddAndMap(item.Key, item.Value);
public void Clear() => _dictionary.Clear();
public bool Contains(KeyValuePair<string, object> item) => _dictionary.ContainsKey(item.Key);
public void CopyTo(KeyValuePair<string, object>[] array, int arrayIndex)
{
foreach (var item in _dictionary)
{
array[arrayIndex++] = item;
}
}
public bool Remove(KeyValuePair<string, object> item) => _dictionary.Remove(item.Key);

public int Count => _dictionary.Count;
public bool IsReadOnly => false;
public void Add(string key, object value) => AddAndMap(key, value);
public bool ContainsKey(string key) => _dictionary.ContainsKey(key);
public bool Remove(string key) => _dictionary.Remove(key);

public bool TryGetValue(string key, out object value)
{
var result = _dictionary.TryGetValue(key, out var outValue);
value = outValue ?? String.Empty;
return result;
}

public object this[string key]
{
get => _dictionary[key];
set => AddAndMap(key, value);
}

public ICollection<string> Keys => _dictionary.Keys;
public ICollection<object> Values => _dictionary.Values;
[YamlMember(Alias = "summary")]
public string? Summary { get; set; }
}

internal partial class NoAuthSecurity : SecurityScheme
Expand Down
18 changes: 18 additions & 0 deletions test/AutoRest.TestServer.Tests/paging.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using System.Threading.Tasks;
using AutoRest.TestServer.Tests.Infrastructure;
using Azure;
Expand Down Expand Up @@ -846,5 +847,22 @@ public void PagingModelsAreHidden()
{
Assert.IsFalse(typeof(ProductResult).IsPublic);
}

[Test]
public void RestClientDroppedMethods()
{
var client = typeof(PagingRestClient);
Assert.IsNull(client.GetMethod ("CreateNextFragmentNextPageRequest", BindingFlags.Instance | BindingFlags.NonPublic), "CreateNextFragmentNextPageRequest should not be generated");
Assert.IsNull(client.GetMethod ("NextFragmentNextPageAsync", BindingFlags.Instance | BindingFlags.NonPublic), "NextFragmentNextPageAsync should not be generated");
Assert.IsNull(client.GetMethod ("NextFragmentNextPage", BindingFlags.Instance | BindingFlags.NonPublic), "NextFragmentNextPage should not be generated");
}

[Test]
public void ClientDroppedMethods()
{
var client = typeof(PagingClient);
Assert.IsNull(client.GetMethod ("NextFragmentAsync", BindingFlags.Instance | BindingFlags.NonPublic), "NextFragmentAsync should not be generated");
Assert.IsNull(client.GetMethod ("NextFragment", BindingFlags.Instance | BindingFlags.NonPublic), "NextFragment should not be generated");
}
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions test/TestServerProjects/paging/Generated/PagingClient.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit ace7084

Please sign in to comment.