Base classes like ScoringFunction should be abstract

[brjohnstmsft]

Library or service name. Azure.Search

Is your feature request related to a problem? Please describe. In the current (Track 1) .NET client library for Azure Cognitive Search, the generated model classes that represent base classes (for example, ScoringFunction) are not abstract, but they should be. This can lead to confusion when customers try to serialize and deserialize the model classes themselves, as in this issue.

We should ensure this is corrected in the Track 2 .NET SDK.

FYI @AlexGhiondea

[maririos]

FYI @tg-msft

[heaths]

I'll discuss with @pakrym but this may be something we have to do manually with the customized model classes. I imagine, in general, it's not easy to tell whether generated base models can be instantiated or only extended. I imagine the latter is more common, but wouldn't expect the former to never occur.

[heaths]

/cc @Yahnoosh

One concern that came up in internal discussions was that if the server returns a different discriminated type, what then? It can't deserialize to anything including the base class because it's abstract. That's actually why @pakrym doesn't mark the classes abstract.

However, I pointed out that IIF the services introduce new types in new api-versions, this shouldn't be a concern. Client libraries, according to guidelines, should use service version enums that are not directly tied to an api-version and don't accept user-input. .NET, for example, uses monotonically increasing version numbers to represent supported api-versions. Combined, no dev should expect an unsupported type from the server in an older version of the library because the type would never be returned. @johanste has pointed out, however, that services often return new types in older api-versions because they don't always bind response models to api-version.

Thus, we may need to leave these models as non-abstract and, instead, on the base models, implement azure/autorest.csharp#650 to support round tripping unknown types.

[brjohnstmsft]

@heaths Is there some way we can address both the deserialization-of-unknown-types issue and the usability issue created by having non-abstract base classes? For example, instead of deserializing unknown properties to a dictionary on the base class, perhaps each hierarchy gets its own UnknownResponse derived type or something similar?

[heaths]

That's a pretty good idea. @pakrym, that could be done generically, right? Maybe an Unknown{Type} model gets generated for each would-be abstract model, and that would have a property bag?

[johanste]

Incidentally, this is one of the recommended patterns for service teams that knew that they had a polymorphic type that they intended to add new child types to.

[pakrym]

It's possible but not very clean. We would need to generate an Unknown{Type} for every polymorphic type that being returned by model or client method.

Would these types be public?

[heaths]

We'd only need one for the base class that you'd otherwise mark abstract. And, yes, I imagine they should be publish. People could dig into the property bag at least. They wouldn't have to be, though. I've seen in some implementations the property bag for unhandled properties are often private just so one can roundtrip.

[pakrym]

We'd only need one for the base class that you'd otherwise mark abstract.

Not really, imagine you have A -> B -> C hierarchy.

And 3 client methods:

A GetA(); B GetB(); C GetC()

You can't just generate an UknownA class and use it as a return value in GetB and GetC you also need UnknownB and UnknownC. Same things with models, if any subclass is directly returned it would need and Unknown{Type}

[heaths]

Fair enough, but how many times is there more than one base class? We have to do something. We're told not to ship "empty classes" and it's confusing for customers if they try to instantiate the base class and pass it along. Have a "few" extra Unknown{Type} classes seems a better option.

/cc @KrzysztofCwalina

[pakrym]

Fair enough, but how many times is there more than one base class?

This is something we would have to figure out. There are at least 3 in storage management.

I agree that we need to figure it out. I think internal classes might be a reasonable solution here.

[heaths]

I think internal classes might be a reasonable solution here.

Could you clarify what you mean? You're agreeing (as an option) that the generator should emit all those classes as needed, but make those classes internal? So we'd just return the base class for roundtripping purposes only.

If that's what you meant, I think that's fine. It won't overload the namespace with a bunch of new types.

[tg-msft]

I'd personally like an interface IExtraProperties { Dictionary<string, object> Properties { get; } } that everything implements explicitly per the discussion in https://github.com/Azure/autorest.csharp/issues/650.

[heaths]

Here's a list of the classes that should be abstract:

• [ ] CharFilter
• [ ] CognitiveServicesAccount
• [ ] DataChangeDetectionPolicy
• [ ] DataDeletionDetectionPolicy
• [ ] LexicalAnalyzer
• [ ] LexicalTokenizer
• [ ] ScoringFunction
• [ ] SearchIndexerSkill
• [ ] Similarity
• [ ] TokenFilter

[heaths]

The generator doesn't handle abstract classes correctly when I added customized models as abstract. Thus, this is blocked on Azure/autorest.csharp#650.

[heaths]

For preview 3 I'll make the constructors private protected, at least.

• [x] CharFilter
• [x] CognitiveServicesAccount
• [x] DataChangeDetectionPolicy
• [x] DataDeletionDetectionPolicy
• [x] LexicalAnalyzer
• [x] LexicalTokenizer
• [x] ScoringFunction
• [x] SearchIndexerSkill
• [x] Similarity
• [x] TokenFilter

[pakrym]

Are all of these classes input-output?

[heaths]

Yes, all input/output.