SynonymMap model should take IEnumerable<string> for synonyms

[heaths]

The generated model for SynonymMap takes a single string for synonyms, which the user would then have to pass with newlines embedded. This "magic formula" approach is something we try to avoid and just make it obvious and idiomatic. Guidelines recommend instead taking an IEnumerable<string> for the parameter and perhaps define the property as an IList<string> to allow adding to it. The de/serializer should handle splitting and joining with newlines accordingly.

/cc @brjohnstmsft

[brjohnstmsft]

This sounds reasonable to me.

@Yahnoosh, can you think of any reason why we might need to keep the synonym map body represented as a single string on the client?

[heaths]

We could provide the transformed string as wel (though I see little value until we need it) since we'll have to eventually de/serialize it anyway. Making callers do that themselves is not very dev-friendly nor idiomatic.

[brjohnstmsft]

My only concern is that we might add synonym map formats in the future that aren’t line-oriented. I’m not the SME for synonyms, so I’ll check and get back to you.

[heaths]

That would be a REST breaking change, since they are already newline delimited. I would suggest a new model type, per our REST guidelines, which we'd need to support in our SDK then anyway.

[brjohnstmsft]

There's a format parameter that currently only accepts one value. The docs say the map itself is newline-delimited regardless, but I'm not sure if that's accurate. If we add a new format that uses a different delimiter, it wouldn't technically be a breaking change.

[heaths]

I wouldn't add a newline for a single value, so that would still work fine.

Changing implementation without changing the API is still a breaking change. If customers had been using newlines and that delimiter changed, their code would break, right? You'd need a new model or, at the very least, change the api-version - either of which we can change the delimiter in code.

/cc @johanste who's been working with service teams to identify and mitigate breaking changes.

[johanste]

Can you add a link to the specific model/REST API that we are considering changing?

[johanste]

In general, if we are being additive for inputs only, then it is generally safer than additive for outputs. But details matter.

[brjohnstmsft]

If the delimiter depends on the value given for the format property, then adding a new format with a different delimiter is not a breaking change because customers would have to opt in to the new format.

There’s also the possibility that customers have existing synonym maps as text files and may want to upload them as-is instead of constructing them programmatically line by line.

The bottom line is, none of us are SMEs on this topic, and further speculation is pointless until I have a chance to sync with more people on my team. 🙂

[brjohnstmsft]

Talked with @Yahnoosh about this. Here's the summary:

• Synonym maps are usually authored as text files, and often imported from another system.
• The REST API documentation is misleading about newline always being the separator -- that actually depends on the format. For now, we only support the SOLR format, and newline is the delimiter for that format.
• Stream makes sense as a way to ingest synonym map files on Create/CreateOrUpdate. However, the single-string property in the SynonymMap object should also be an option, if for no other reasons that demos/samples and the Get/List APIs.

Please let me know if you have any questions.

[johanste]

If the delimiter depends on the value given for the format property, then adding a new format with a different delimiter is not a breaking change because customers would have to opt in to the new format.

Based on the fact that the synonym map can be returned to older clients (per https://github.com/Azure/azure-rest-api-specs/blob/8919f8072121d11f0bb61f1c5d02f2f4063d4f48/specification/search/data-plane/Microsoft.Azure.Search.Service/preview/2016-09-01-preview/searchservice.json#L704), it is very likely a change that requires a new API version per Azure's versioning rules.

Let's follow up ones we have more specifics on the proposed change.

[brjohnstmsft]

@johanste True, I forgot about the round-tripping rule.

[heaths]

What about taking a Stream + format, or either a string or array of strings with SOLR assumed? Looking through a few examples, there are some that pass \n like https://docs.microsoft.com/en-us/rest/api/searchservice/create-synonym-map#-examples--.

public class SynonymMap
{
  public SynonymMap(string format, Stream stream, Encoding encoding = null);
  public SynonymMap(string value) : this(new[] { value });
  public SynonymMap(IEnumerable<string> value);

  public string Name { get; }
  public string Format { get; }
  public string Value { get; }
  public EncryptionKey EncryptionKey { get; set; }
  public string ETag { get; set; }
}

Optionally, if text is always expected (safe assumption, or not?), we could take in a TextReader and not have to worry about encoding directly (the TextReader implementation would).

Either way, the content would be decoded into a string for Value and sending across the wire. I do question whether there's much value in the second constructor above, though.

[brjohnstmsft]

@heaths Having a constructor that takes a Stream or TextReader makes sense. I think it's a pretty safe assumption that it will always be text, so TextReader is probably fine.

Taking format as a parameter is problematic right now because there is only one allowable value (it should be an enum and not a string BTW). We hide it in Track 1 because it's pointless for users to have to pass the same value all the time. If we can add it as an additional parameter (or constructor overload) later, IMO we can leave it out for now.

I would actually question the value of the third constructor rather than the second. If the map isn't coming from a file, it's probably being pasted from somewhere else. Constructing it programmatically line-by-line is useful for samples, but that's about it. CC @Yahnoosh who can back me up on this.