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

WhereAnd appears to cause Acuminator to not warn about improper argument length on Select #409

Open
ShadowFiendZX opened this issue Oct 29, 2021 · 1 comment
Labels
bug Something isn't working static analysis Items related to static code analysis

Comments

@ShadowFiendZX
Copy link
Collaborator

Recently we ran into an issue caused by us not having passed in enough arguments to a Select. It took a while to find, as Acuminator wasn't showing a warning in this specific use case.

We'd originally had a series of conditions, but we'd broken one off and put it in a WhereAnd applied under a certain circumstance. However, we forgot to remove the condition from the original Select declaration. We found that specifically when WhereAnd is involved, Acuminator may not show the warning.

Here is some sample code that can be added to any graph referencing PX.Objects.AP. We also found that it's both parts of the if/else that Acumatica doesn't show the warning.

//Test to see if it was the if/else that was causing the problem.
protected virtual void __(Events.RowInserting<APInvoice> e)
{
	if (e.Row != null)
	{
		PXSelectBase<APTran> cmd = new PXSelectReadonly
		<
			APTran,
			Where
			<
				APTran.tranType, Equal<Required<APTran.tranType>>,
				And<APTran.refNbr, Equal<Required<APTran.refNbr>>,
				And<APTran.lineNbr, Equal<Required<APTran.lineNbr>>>>
			>
		>(Base);

		bool b = false;

		if (b)
		{
		}
		else
		{
                        //Warning shows just fine
			cmd.Select(e.Row.DocType, e.Row.RefNbr);
		}
	}
}

protected virtual void __(Events.RowSelecting<APInvoice> e)
{
	if (e.Row != null)
	{
		PXSelectBase<APTran> cmd = new PXSelectReadonly
		<
			APTran,
			Where
			<
				APTran.tranType, Equal<Required<APTran.tranType>>,
				And<APTran.refNbr, Equal<Required<APTran.refNbr>>,
				//Accidentally left this And here when WhereAnd was written.
				And<APTran.lineNbr, Equal<Required<APTran.lineNbr>>>>
			>
		>(Base);

		using (new PXConnectionScope())
		{
			bool b = false;

                        //Warning doesn't show in either case.
			if (b)
			{
				cmd.WhereAnd
				<
					Where
					<
						APTran.lineNbr, Equal<Required<APTran.lineNbr>>
					>
				>();

				//Wrong number of arguments passed to see if it was only in the else that Acuminator didn't show the warning.
				cmd.Select(e.Row, e.Row.DocType);
			}
			else
			{
				cmd.Select(e.Row.DocType, e.Row.RefNbr);
			}
		}
	}
}
@SENya1990
Copy link
Contributor

Hi @ShadowFiendZX . I think it is a bug when there is no warning in the else block in your example. But the rest is an expected behavior, including not showing warning in the if block.

The current PX1015 analyzer detects methods that modify BQL query such as WhereAnd, WhereNot, WhereNew and some other methods. Here's the list:
https://github.com/Acumatica/Acuminator/blob/0aa7c1946fd40c221946a7ddd94e885001ca255a/src/Acuminator/Acuminator.Utilities/Roslyn/BqlModifyingMethods.cs

The current implementation detects calls to these methods, but it doesn't calculate corrections from them. Instead it just stops the check so no error would be produced. If you modify BQL dynamically then the query is not checked.

It's not impossible to calculate the number of parameters in some cases but it is impossible to do this in general.
For example, what if you have a code like this:

public static void TryCalculateParametersForThis<Operand, Comparison>(List<Type> conditions)
{
   var query = new PXSelect<APTran,
			             Where<APTran.tranType, Equal<Required<APTran.tranType>>,
				        And<APTran.refNbr, Equal<Required<APTran.refNbr>>,
				       And<APTran.lineNbr, Equal<Required<APTran.lineNbr>>>>>>(Base);
   
   foreach (Type condition in conditions)
   {
       query.WhereAnd(condition);
   }
   
  query .Select(e.Row.DocType, e.Row.RefNbr);
}

You can find similar example for a generic WhereAnd method.
It is possible to try to distinguish such cases but I'm afraid we won't implement such analysis or do it in the distant future. It should be a complex analysis that will require a lot of time to implement. At the same time this case is really rare. Almost all BQLs don't use BQL modifying methods. In fact, many of them are static.

@SENya1990 SENya1990 added the bug Something isn't working label Oct 30, 2021
@SENya1990 SENya1990 added the static analysis Items related to static code analysis label Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working static analysis Items related to static code analysis
Projects
None yet
Development

No branches or pull requests

2 participants