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.

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

New rule for Web Apis #149

Closed
PaymonK opened this issue May 22, 2018 · 5 comments
Closed

New rule for Web Apis #149

PaymonK opened this issue May 22, 2018 · 5 comments
Assignees
Milestone

Comments

@PaymonK
Copy link
Contributor

PaymonK commented May 22, 2018

If we have a web api action method such as:

[HttpPost, Route("myUrl")]
public async Task<IActionResult> MyApi(SomeType param1)
{
   ....
}

If the following conditions are met:

  • The method's Http action is HttpPost, HttpPut, HttpPatch or HttpDelete
  • The parameter type is a type defined in the application (i.e. not in the .NET framework)
  • The argument does not have any [FromXXX] attribute (such as FromQuery, FromRoute, ....)

Then show a warning to say: "For a {Post} Web Api, specify the parameter binding source explicitly. Did you mean to add [FromBody]?"

Instead of {Post} use the actual http verb.

@Foroughi
Copy link
Contributor

@Foroughi
Copy link
Contributor

Foroughi commented May 29, 2018

Here are some details of the new rule :

General Details:

  • ID : 182
  • Category : Design
  • Message : For a {0} Web Api, specify the parameter binding source explicitly. Did you mean to add [FromBody]?
  • Severity : Warning
  • Package : GCop.Common

Technical Details :

The Rule will look for a method with the following specs :

  • Should be public
  • Should be decorated with an Http action like HttpPost
  • Should have at least one parameter with a user-defined type which has not binding source attribute

Test Result :

  • [HttpPost] private void TestMethod(DefinedUserClass x) {}
  • [HttpPost] public void TestMethod(DefinedUserClass x) {}
  • [HttpPost] public void TestMethod(int x) {}
  • [HttpPost] public void TestMethod([FromForm] DefinedUserClass x) {}
  • [HttpPost] public void TestMethod(int x , DefinedUserClass y) {}

In case of any changes needs to be done on this rule please let me know.

@marjanjavid
Copy link
Collaborator

marjanjavid commented Feb 28, 2019

This rule is not working. It just warn when we don't defined the parameter type.
In the picture below we don't see any warning about GCop182 because the parameter is defined by programmer

182

but here after removing the class we can see the error
1822

This happen because of line 62 in the rule is written like this
public bool IsUserDefinedType(SyntaxNodeAnalysisContext context, ParameterSyntax param) => context.SemanticModel.GetTypeInfo(param.Type).Type?.Kind == SymbolKind.ErrorType;

Also it does not warn about string parameters
@Karvan Am I write? Can I edit this rule?

@marjanjavid marjanjavid added the bug Something isn't working label Feb 28, 2019
@marjanjavid marjanjavid self-assigned this Feb 28, 2019
@marjanjavid
Copy link
Collaborator

marjanjavid commented Mar 1, 2019

This is Edited on This

Test Result :

  • [HttpPost] private void TestMethod(DefinedUserClass x) {}
  • [HttpPost] public void TestMethod(DefinedUserClass x) {}
  • [HttpPost] public void TestMethod(int x , DefinedUserClass y) {}
  • [HttpPost] public void TestMethod(object x) {}
  • [HttpPost] public void TestMethod(int x) {}
  • [HttpPost] public void TestMethod(string x) {}
  • [HttpPost] public void TestMethod(System.DateTime x) {}
  • [HttpPost] public void TestMethod(Decimal x) {}
  • [HttpPost] public void TestMethod(bool x) {}
  • [HttpPost] public void TestMethod(char x) {}
  • [HttpPost] public void TestMethod(double x) {}
  • [HttpPost] public void TestMethod(single x) {}
  • [HttpPost] public void TestMethod([FromForm] DefinedUserClass x) {}

@Karvan I have a question :
As mentioned here

If the parameter is a "simple" type, Web API tries to get the value from the URI. Simple types include the .NET primitive types (int, bool, double, and so forth), plus TimeSpan, DateTime, Guid, decimal, and string, plus any type with a type converter that can convert from a string.

we should not warn about TimeSpan and Guid parameters. But I couldn't find anyway to add these types to our rule. Because none of theme exist in SpecialType enum.

Also I saw Roslyn consider Nullable<T> and Enum as primitive types, while these were not mentioned in the above link.

So @PaymonK @Karvan @Foroughi I am not sure about Nullable<T> and Enum and TimeSpan and Guid. please help me to take care of these.

@marjanjavid
Copy link
Collaborator

The changes are available on GCop2.6.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants