Add Diagnostic to ensure that only SelectBase derivations are used in PXProjections / ensure that PXSelectBase derivations aren't used

[ShadowFiendZX]

One mistake I've seen in many code reviews is that some of my coworkers will use PXSelectGroupBy, etc in their PXProjections instead of the proper Select
While this does lead to a runtime issue, namely an error along the lines of No parameterless constructor defined for this object., it'd probably be easier for a lot of developers if Acuminator had a diagnostic about this.

[SENya1990]

Hi @ShadowFiendZX , thanks for a suggestion. I will create a change request in our internal bug tracker for such diagnostic.
I think it will be helpful. @dropsonic what do you think?

Could you please provide a more detailed example of code? A snippet with incorrect code and a snippet with a correct one will be really helpful.
Also should such check apply only to PXProjection attribute? Are there other attributes that allow similar mistake?

[ShadowFiendZX]

Here's some code for PXProjections, as well as PXDBScalar. I might've gone a bit overboard on the PXProjection ones...
My thought was that while it may be possible for Acuminator to check if the Type derives from SearchBase, it might be beneficial to write out all the possible derivations manually in case the type checking is problematic, or changes. However, if this isn't likely to become an issue, the sample code can be pared down.

PXDBCalced can have similar issues, although it's definitely a lot more complicated, since you can use things like IBqlFunction, IBqlCreator, etc.

Some other possible checks to make that should probably get put into a separate issue/fix would be :

• Ensuring the Type passed into PXDBCalced is proper for the field. This includes ensuring that a developer did not use "?" (Nullable<>) as this causes issues. Potentially also ensuring that the user didn't put a DAC or some unaccepted Type (any other class/struct/etc such as Dictionary<,>)
  Wrong : PXDBCalced(typeof(...), typeof(int?)) Correct : PXDBCalced(typeof(...), typeof(int));
• In PXProjections, ensuring that BqlTable and BqlField are valid for the query. This would definitely be complicated, as there are a number of DACs in Acumatica that derive from other DACs but don't redeclare the field class, and the logic would have to account for Extension Fields.

Temp.zip

[SENya1990]

@ShadowFiendZX thanks for the detailed feedback. I created a change request to add such diagnostic.

Regarding the extra checks:

Ensuring the Type passed into PXDBCalced is proper for the field. This includes ensuring that a developer did not use "?" (Nullable<>) as this causes issues. Potentially also ensuring that the user didn't put a DAC or some unaccepted Type (any other class/struct/etc such as Dictionary<,>)
Wrong : PXDBCalced(typeof(...), typeof(int?)) Correct : `PXDBCalced(typeof(...), typeof(int));

Did you encounter mistakes like you described in this one?
It may be good to check but there is a lot of similar things in Acumatica Framework that would be good to check and unfortunately we lack resources to implement diagnostics for all of them. We usually put priority on things people encountered in Acumatica and complained about.

Regarding the second extra check, I didn't understand clearly the details of the check. Could you please give more details? What do you mean by "valid for the query"?

[ShadowFiendZX]

When you say things people encountered in Acumatica and complained about. do you mean things people encountered while developing within the Acumatica Framework?

If so, the mistake with the type when using PXDBCalced is one my coworkers and I run into often. In that we each don't often use the Attribute, so when we do, we often second guess whether or not the Type should be nullable until we either compile and try it, or check through our other projects for occurrences. All in all this can take 1 to several minutes depending on reload times, or how quickly we can find a project that has a usage, so I'm not sure how life changing it'd be, but it'd be nice if Acuminator warned us.

As for the second extra check for PXProjections I can give you some reasonable examples, on top of some outlandish ones that I'd originally hinted at.

Case 1 :
Let's say you have a PXProjection like this :

	[Serializable]
	[PXProjection
	(
		typeof
		(
			Select2
			<
				APInvoice,
				InnerJoin
				<
					APTran,
					On
					<
						APTran.tranType, Equal<APInvoice.docType>,
						And<APTran.refNbr, Equal<APTran.refNbr>>
					>
				>
			>
		)
	)]
	[PXHidden]
	public class SomeProjection : IBqlTable
	{

		#region DocType
		public abstract class docType : BqlString.Field<docType>
		{
		}
		[PXDBString(3, IsFixed = true, IsKey = true, BqlTable = typeof(APInvoice), BqlField = typeof(APInvoice.docType))]
		public virtual string DocType { get; set; }
		#endregion

		#region RefNbr
		public abstract class refNbr : BqlString.Field<refNbr>
		{
		}
		[PXDBString(15, IsUnicode = true, IsKey = true, InputMask = ">CCCCCCCCCCCCCCC", BqlTable = typeof(APInvoice), BqlField = typeof(APInvoice.refNbr))]
		public virtual string RefNbr { get; set; }
		#endregion

		#region TranAmt
		public abstract class tranAmt : BqlDecimal.Field<tranAmt>
		{
		}
		[PXDBBaseCury(BqlTable = typeof(APTran), BqlField = typeof(APTran.tranAmt))]
		public virtual decimal? TranAmt { get; set; }
		#endregion

	}

Either as you're developing it, or later down the line you decide you no longer want/need to have APInvoice in the query so you change the query :

	[Serializable]
	[PXProjection
	(
		typeof
		(
			Select
			<
				APTran
			>
		)
	)]
	[PXHidden]
	public class SomeProjection : IBqlTable
	{

		#region DocType
		public abstract class docType : BqlString.Field<docType>
		{
		}
		[PXDBString(3, IsFixed = true, IsKey = true, BqlTable = typeof(APInvoice), BqlField = typeof(APInvoice.docType))]
		public virtual string DocType { get; set; }
		#endregion

		#region RefNbr
		public abstract class refNbr : BqlString.Field<refNbr>
		{
		}
		[PXDBString(15, IsUnicode = true, IsKey = true, InputMask = ">CCCCCCCCCCCCCCC", BqlTable = typeof(APInvoice), BqlField = typeof(APInvoice.refNbr))]
		public virtual string RefNbr { get; set; }
		#endregion

		#region TranAmt
		public abstract class tranAmt : BqlDecimal.Field<tranAmt>
		{
		}
		[PXDBBaseCury(BqlTable = typeof(APTran), BqlField = typeof(APTran.tranAmt))]
		public virtual decimal? TranAmt { get; set; }
		#endregion

	}

Ignoring the simplicity of this example, let's pretend that this is a large PXProjection with at least 20 fields. In this case, for whatever reason, you forget to review all your fields and you don't realize that you left references to APInvoice :

[PXDBString(3, IsFixed = true, IsKey = true, BqlTable = typeof(APInvoice), BqlField = typeof(APInvoice.docType))]
public virtual string DocType { get; set; }

This will compile, but will cause issues during runtime.

Case 2 :
Let's say you have some common logic that applies to multiple DACs, so you create an interface, and then write your logic based off of it :

	public interface IHistoryTransaction
	{

		public string DrCr { get; }
		public decimal? Amt { get; }
		public decimal? CuryAmt { get; }

	}

Now you write a PXProjection using APTran, and implement your interface :

	[Serializable]
	[PXProjection
	(
		typeof
		(
			Select4
			<
				APTran,
				Aggregate
				<
					GroupBy<APTran.drCr,
					Sum<APTran.tranAmt,
					Sum<APTran.curyTranAmt>>>
				>
			>
		)
	)]
	[PXHidden]
	public class APTranHistory : IBqlTable, IHistoryTransaction
	{

		#region DrCr
		public abstract class drCr : BqlString.Field<drCr>
		{
		}
		[PXDBString(1, IsFixed = true, IsKey = true, BqlTable = typeof(APTran), BqlField = typeof(APTran.drCr))]
		public virtual string DrCr { get; set; }
		#endregion

		#region Amt
		public abstract class amt : BqlDecimal.Field<amt>
		{
		}
		[PXDBDecimal(BqlTable = typeof(APTran), BqlField = typeof(APTran.tranAmt))]
		public virtual decimal? Amt { get; set; }
		#endregion

		#region CuryAmt
		public abstract class curyAmt : BqlDecimal.Field<curyAmt>
		{
		}
		[PXDBDecimal(BqlTable = typeof(APTran), BqlField = typeof(APTran.curyTranAmt))]
		public virtual decimal? CuryAmt { get; set; }
		#endregion

	}

Either during initial development, or later down the line, you find out you need the logic to apply data from ARTran too. So to save some keystrokes you copy the APTran PXProjection, and modify it :

	[Serializable]
	[PXProjection
	(
		typeof
		(
			Select4
			<
				ARTran,
				Aggregate
				<
					GroupBy<ARTran.drCr,
					Sum<ARTran.tranAmt,
					Sum<ARTran.curyTranAmt>>>
				>
			>
		)
	)]
	[PXHidden]
	public class ARTranHistory : IBqlTable, IHistoryTransaction
	{

		#region DrCr
		public abstract class drCr : BqlString.Field<drCr>
		{
		}
		[PXDBString(1, IsFixed = true, IsKey = true, BqlTable = typeof(APTran), BqlField = typeof(APTran.drCr))]
		public virtual string DrCr { get; set; }
		#endregion

		#region Amt
		public abstract class amt : BqlDecimal.Field<amt>
		{
		}
		[PXDBDecimal(BqlTable = typeof(APTran), BqlField = typeof(APTran.tranAmt))]
		public virtual decimal? Amt { get; set; }
		#endregion

		#region CuryAmt
		public abstract class curyAmt : BqlDecimal.Field<curyAmt>
		{
		}
		[PXDBDecimal(BqlTable = typeof(APTran), BqlField = typeof(APTran.curyTranAmt))]
		public virtual decimal? CuryAmt { get; set; }
		#endregion

	}

Let's pretend that you didn't use Find and Replace, or that there is some other reason you didn't end up changing all the references to be proper :

[PXDBString(1, IsFixed = true, IsKey = true, BqlTable = typeof(APTran), BqlField = typeof(APTran.drCr))]
public virtual string DrCr { get; set; }

Again this would compile, but would error during runtime.

These two use cases are without a doubt a result of a lack of due diligence, and more often than not I see it happen with some of our less experienced devs. However, even the most experienced dev can make mistakes sometimes, and this mistake is sometimes hard to miss by reviewers depending on how big the DAC is.

Implementing a check for this seems to me like it'd be difficult, although maybe you guys would know otherwise. Parsing through all the possible attributes/types on properties is where I see the most difficulty due to how many possibilities there are.
IE

• BqlTable/BqlField in DB Attributes
• Default, or Operand values in Switch/Case in a PXDBCalced. (Not to mention any other possible Type, including 3rd party BQL Types)
• Ensuring the query on PXDBScalar works for the query of the PXProjection

For the BqlField on a DB Attribute there's the possibility for devs to pass in PXCacheExtensions. I'm also not sure what would happen in a case where Acumatica has a derived DAC that doesn't redeclare the field class.
For example, ARPayment does not redeclare the aRAccountID field class. I'd imagine it'd take extra effort to handle cases such as that.

The more outlandish examples would be cases where a developer might pass something ridiculous in like this :

[PXProjection(typeof(HashSet<int>))]

[PXDBCalced(typeof(string), (typeof(int))]

[PXDBScalar(typeof(Dictionary<string, string>))]

While very unlikely to happen, you never know what someone might try to do. I'd imagine that dealing with the valid use cases would error about these use cases depending on the implementation, but either way it might be worth having a diagnostic about in the off chance that some 3rd party developer tries something naively and doesn't understand why it doesn't work.

Don't take all this to mean that I think these are high priority. They're rather small things that should be found during testing/code review, but I'm sure you know how that goes sometimes.

[SENya1990]

Hi @ShadowFiendZX . That's a great feedback. If only we received more things like this!
There is a lot of things, so I'll answer them separately.

When you say things people encountered in Acumatica and complained about. do you mean things people encountered while developing within the Acumatica Framework?

Yes, that's exactly what I mean. Acuminator is not the part of the main product, so not much resources are spent on its development. So, we have to put off more rare error scenarios. That's why there are several key factors for picking a diagnostic to implement:

• How frequently an error is encountered?
• How sever is the introduced error? The most sever ones are actually the one that will not fail your application during the run-time but continue to run silently. They may cause a data consistency issue under some specific circumstances. For example, PX1008:
  https://github.com/Acumatica/Acuminator/blob/dev/docs/diagnostics/PX1008.md
• Of course, the cost of implementation is also considered

I think, if you and your teammates encounter the problem with passed types that it worth creating such check. It shouldn't be very difficult too. I will add an item to our bug tracker.

[SENya1990]

Regarding the validation of DB query passed to attributes - I have already created and item to check if it is derived from SelectBase. This should cover not only other types of BQL queries but also all other types like in your example:

[PXProjection(typeof(HashSet<int>))]

If I understand correctly other examples are all related to only one check. Do I understand correctly that the validation of projection DACs you mentioned is a check of all tables referenced in projection DAC fields in BqlField or BqlTable properties to see if they are also present in the projection BQL query?

[ShadowFiendZX]

Do I understand correctly from your examples that the validation diagnostic you mean is a check that all tables referenced in projection DAC fields in BqlField or BqlTable properties should be also present in the projection BQL query?
Yes that is the secondary check I was talking about.

[SENya1990]

@ShadowFiendZX something strange with Github behavior, I can't see my like for the comment and my answers got deleted

[SENya1990]

Seems like it's working ok now.

Regarding the projection checks - I will create a change request for such diagnostic, I think it will be useful to have a suite of validations for projection DACs. It is possible to check during the compile time. I think it should be possible to check it for cache extension although I need to check if DB queries passed to projection attributes are available from metadata.