New rule: Noun in operationId must be different from Noun in Model definition

[veronicagg]

M1002: The NOUN in the OperationId "NOUN_VERB" MUST not match the name of the Model definition, to avoid a collision in the namespace, use the plural form of NOUN in the operationId.
Category: SDK Violation
Severity: Error

[dsgouda]

This is going to be rather difficult to implement. Even if we validate that the "NOUN" in "NOUN_VERB" does not match with any of the defined models, we won't be able to clearly suggest a plural form for the model (the classic Factory and Factories example).
The best we can do is throw a more generic error that simply states that the NOUN provided conflicts with one of the model definitions, and the author should therefore modify it.
Tagging @sarangan12 since he ran into a similar issue with one of the rules.
Alternatively, I see this as a possible solution
https://msdn.microsoft.com/en-us/library/system.data.entity.design.pluralizationservices.pluralizationservice(v=vs.110).aspx

[veronicagg]

@dsgouda I agree, let's not try to get smart suggesting a specific name, let's just point out the conflict and suggest in the error text, that they could use the plural form of the NOUN to avoid the conflict.

[matthchr]

@vishrutshah suggested I comment here.

The issue with the check right now is that you assume that there is actually a conflict when there may not be. For example this operation:
Certificate_Get and an entity Certificate do not actually collide in most languages.

In C# you get CertificateOperations.Get which doesn't collide with the model Certificate.
In Python you get CertificateOperations.Get which doesn't collide with the model Certificate
In Java you get CertificateOperations.Get which doesn't collide with the model Certificate.

I think you get the point =)

IMO this rule is too generator specific and you're making an assumption about the structure of the generated code which isn't even true. The only language that I know of where this actually conflicts is Node, and (IMO) the fix isn't to flag errors in the generator the fix is to make nodes type CertificateOperations instead of Certificate like it is now.

This avoids the awkward "add an s" (since then it's CertificatesOperations everywhere which reads pretty awkwardly), and it also is just more correct to boot (CertificateOperations is more clear about what the class is actually doing than Certificates).

[vishrutshah]

@dsgouda & @veronicagg Feel free to throw in your thoughts here...

[dsgouda]

@matthchr CertificateOperations is actually a name resolved by AutoRest when it sees conflicting names, for eg. if you were to name model and operation ids as lets say Cert and Certificate_get, we would see Certificate.cs with all the operations.
@olydis can verify this for us.
The very point is that we wish to maintain consistency between all client names i.e. generate all clients named as Certificate.cs without having to rely on AutoRest to resolve conflicting names. Hope this is helpful.

[matthchr]

@dsgouda No it's not - the behavior depends on language in AutoRest already.

Look at JobOperations.cs for example. We do not have a model named job. It just generates it with the operations suffix as above by default.

Again, as far as I know the behavior is like so:
Python, C#, and Java all append "Operations" by default.
Node.js does not.

Please see this issue, which I filed a year ago pointing out the inconsistency.

The point being -- since the code generators themselves aren't even consistent, I am not sure how you can give this warning consistently across all languages... and if you were to give a warning, since most code generators append Operations by default, you should probably suggest based on that rather than assuming that the generator doesn't append anything (since the majority do).

edit
I don't know what Go/Ruby do, as those generators weren't done when I last looked.

Also looking through the filed issue for node I even suggested a linter rule (this linter rule), so I'm contradicting myself a bit here =P In any case, a discussion should be had about what the right rule is because I'm not 100% sure that it's the one you currently have... the issue with the one you have is while it's technically correct it's also super easy to hit (because of course every top level entity is going to have an associated operations class...)

[olydis]

AFAIK it's how @dsgouda describes for the "vanilla" C# generator and as @matthchr describes for the Azure/ARM flavor of the generator

[dsgouda]

So, we do append Operations to the Noun in Noun_Verb by default when azure-arm flag is passed during code generation.
This validation rule would make more sense if we are to actually generate code without the flag , which no sdk does AFAIK.
Based on this, I agree with @mattchr and we should remove this validation rule altogether.
@veronicagg thoughts?

[matthchr]

@dsgouda @veronicagg I see that this rule is still around -- what are the plans related to it?

[dsgouda]

This was slated to be removed might have fallen through the cracks.

[dsgouda]

Issue opened here since all validation related code will be moved out of this repo.