Eliminate hardcoded DataPortal_XYZ method names

[rockfordlhotka]

Today the DataPortal_XYZ and Child_XYZ method names are hardcoded into the data portal. They should not be hardcoded.

In short, people should be able to come up with their own naming convention for these methods, and the data portal should invoke the appropriate methods.

UPDATE: This comment has a detailed list of the options being considered.

[JasonBock]

I'm biased here, but I like it 👍 . Honestly, this should be the way it was done from Day 1, and I can see this same approach being used in other places in CSLA (rule registration is another, but let's just start with these for now).

One thing I'd like to see is what the difference is with method discovery via attributes vs a well-known name. The difference would probably be small and I believe CSLA caches the method once it's found, but it would be fun to Benchmark.NET that to see what it is.

[JasonBock]

I also think having an analyzer here would help people easily move to this attribute approach (assuming it's better all around). We could offer adding the attribute to operations automatically, and then at some point, the internals of CSLA remove that method lookup path, but the analyzer would stick around for a while to help developers easily update code.

[JasonBock]

Before we go too far down this road, I decided to do some perf testing. First, I defined a [FetchAttribute]:

[AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = false)]
public sealed class FetchAttribute
  : Attribute
{ }

I created a bunch of classes that contained either methods decorated with a [FetchAttribute]:

[Fetch]
void Method1(Guid id) { }

or a well-known name:

void DataPortal_Fetch(int id) { }

I created classes with 5, 10, 20 and 50 methods. A class either contained 2 attributed methods, or 2 well-known named methods. The lookup code looked like this:

[Benchmark]
public int FindMethodCountWith5UsingIsDefined() =>
  typeof(ClassWith5MethodsAndAttribute)
    .GetMethods(BindingFlags.Instance | BindingFlags.NonPublic)
    .Where(_ => _.IsDefined(typeof(FetchAttribute))).Count();

[Benchmark]
public int FindMethodCountWith5UsingGetCustomAttribute() =>
  typeof(ClassWith5MethodsAndAttribute)
    .GetMethods(BindingFlags.Instance | BindingFlags.NonPublic)
    .Where(_ => _. GetCustomAttribute<FetchAttribute>() != null).Count();

[Benchmark]
public int FindMethodCountWith5UsingWellKnownName() =>
  typeof(ClassWith5MethodsAndWellKnownName)
    .GetMethods(BindingFlags.Instance | BindingFlags.NonPublic)
    .Where(_ => _.Name == FetchMethod).Count();

There are various ways to determine if a member has an attribute - the two I've included in my tests seem to be the best approaches. Here are the results:

BenchmarkDotNet=v0.11.4, OS=Windows 10.0.17763.316 (1809/October2018Update/Redstone5)
Intel Core i7-7820HQ CPU 2.90GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=2.2.105
  [Host]     : .NET Core 2.2.3 (CoreCLR 4.6.27414.05, CoreFX 4.6.27414.05), 64bit RyuJIT
  DefaultJob : .NET Core 2.2.3 (CoreCLR 4.6.27414.05, CoreFX 4.6.27414.05), 64bit RyuJIT

|                                       Method |        Mean |      Error |    StdDev | Gen 0/1k Op | Gen 1/1k Op | Gen 2/1k Op | Allocated Memory/Op |
|--------------------------------------------- |------------:|-----------:|----------:|------------:|------------:|------------:|--------------------:|
|           FindMethodCountWith5UsingIsDefined |  3,134.9 ns |  62.158 ns | 116.75 ns |      0.1335 |           - |           - |               576 B |
|  FindMethodCountWith5UsingGetCustomAttribute |  8,642.1 ns | 162.365 ns | 166.74 ns |      0.3204 |           - |           - |              1344 B |
|       FindMethodCountWith5UsingWellKnownName |    371.8 ns |   7.312 ns |  11.17 ns |      0.0610 |           - |           - |               256 B |

|          FindMethodCountWith10UsingIsDefined |  4,127.5 ns |  82.275 ns |  72.93 ns |      0.1526 |           - |           - |               656 B |
| FindMethodCountWith10UsingGetCustomAttribute | 13,071.9 ns | 329.667 ns | 292.24 ns |      0.4425 |           - |           - |              1904 B |
|      FindMethodCountWith10UsingWellKnownName |    477.3 ns |   9.549 ns |  16.47 ns |      0.0801 |           - |           - |               336 B |

|          FindMethodCountWith20UsingIsDefined |  6,128.2 ns | 112.359 ns |  99.60 ns |      0.1907 |           - |           - |               816 B |
| FindMethodCountWith20UsingGetCustomAttribute | 21,397.1 ns | 417.612 ns | 598.93 ns |      0.7019 |           - |           - |              3024 B |
|      FindMethodCountWith20UsingWellKnownName |    692.1 ns |  13.772 ns |  29.35 ns |      0.1173 |           - |           - |               496 B |

|          FindMethodCountWith50UsingIsDefined | 11,305.4 ns | 175.726 ns | 164.37 ns |      0.3052 |           - |           - |              1296 B |
| FindMethodCountWith50UsingGetCustomAttribute | 46,726.1 ns | 798.896 ns | 667.11 ns |      1.4648 |           - |           - |              6384 B |
|      FindMethodCountWith50UsingWellKnownName |  1,283.9 ns |  13.557 ns |  12.02 ns |      0.2327 |           - |           - |               976 B |


BenchmarkDotNet=v0.11.4, OS=Windows 10.0.17763.316 (1809/October2018Update/Redstone5)
Intel Core i7-7820HQ CPU 2.90GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
  [Host]     : .NET Framework 4.7.2 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.3190.0
  DefaultJob : .NET Framework 4.7.2 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.3190.0

|                                       Method |        Mean |        Error |       StdDev | Gen 0/1k Op | Gen 1/1k Op | Gen 2/1k Op | Allocated Memory/Op |
|--------------------------------------------- |------------:|-------------:|-------------:|------------:|------------:|------------:|--------------------:|
|           FindMethodCountWith5UsingIsDefined |  4,607.2 ns |    26.645 ns |    22.250 ns |      0.1831 |           - |           - |               792 B |
|  FindMethodCountWith5UsingGetCustomAttribute | 13,134.5 ns |   130.919 ns |   122.461 ns |      0.4425 |           - |           - |              1896 B |
|       FindMethodCountWith5UsingWellKnownName |    339.8 ns |    10.172 ns |     9.515 ns |      0.0625 |           - |           - |               264 B |

|          FindMethodCountWith10UsingIsDefined |  5,849.3 ns |   104.554 ns |    97.800 ns |      0.2365 |           - |           - |               992 B |
| FindMethodCountWith10UsingGetCustomAttribute | 18,868.8 ns |   188.312 ns |   166.934 ns |      0.6409 |           - |           - |              2696 B |
|      FindMethodCountWith10UsingWellKnownName |    448.2 ns |     5.050 ns |     4.477 ns |      0.0815 |           - |           - |               344 B |

|          FindMethodCountWith20UsingIsDefined |  7,676.3 ns |   140.295 ns |   117.153 ns |      0.3204 |           - |           - |              1392 B |
| FindMethodCountWith20UsingGetCustomAttribute | 30,805.5 ns |   433.344 ns |   405.350 ns |      0.9766 |           - |           - |              4297 B |
|      FindMethodCountWith20UsingWellKnownName |    699.8 ns |     8.732 ns |     7.740 ns |      0.1202 |           - |           - |               504 B |

|          FindMethodCountWith50UsingIsDefined | 13,983.6 ns |   153.433 ns |   143.521 ns |      0.6104 |           - |           - |              2592 B |
| FindMethodCountWith50UsingGetCustomAttribute | 65,309.8 ns | 1,275.028 ns | 1,612.502 ns |      2.0752 |           - |           - |              9097 B |
|      FindMethodCountWith50UsingWellKnownName |  1,216.5 ns |    23.314 ns |    21.808 ns |      0.2327 |           - |           - |               984 B |

As you can see, in both .NET Core and .NET Framework, the attribute approach (with IsDefined()) is an order of magnitude slower than the well-known name approach, and consumes 2 to 3 times more in memory. Notice that .NET Core is overall better than .NET Framework :).

Now, all is not lost:

• It's definitely possible my tests are skewed in different ways that I don't realize. If there's a technique I missed to do attribute discovery, or a different way works better, please let me know.
• There are a couple of interesting pieces of work that's been going on in CoreCLR. These don't appear to be part of .NET Core 2.2 but might be part of .NET Core 3.0. These would not be going into .NET Framework 4.8 as far as I can tell. It's also not clear if they would help out with the perf tests I did above.
  • Improve CustomAttribute - Improve CustomAttribute dotnet/coreclr#21832
  • Optimization: Reduce many array allocations in GetCustomAtttributes()'s inherited path - Optimization: Reduce many array allocations in GetCustomAtttributes()'s inherited path dotnet/coreclr#20779
  • Optimization: Further reduce allocations in GetCustomAtttributes() paths - Optimization: Further reduce allocations in GetCustomAtttributes() paths dotnet/coreclr#20795
• The design of this approach should also be considered. It may not perform as well, but we're also talking tens of microseconds and thousands of bytes, not tens of seconds and gigabytes. These differences shouldn't be dismissed, but is the trade-off that the attribute design is "better"? Or is a well-known name sufficient? The current approach in CSLA is prone to spelling mistakes, where an attribute would be safe from that. Is that a big deal?

One last thing to consider is that we may want to find a consistent way to handle features in CSLA. That is, using attributes for everything (e.g. rule registration, operation definitions, etc.) or use descriptor similar to what is done with managed backing fields. @rockfordlhotka and I had an initial discussion about this last night, and there needs to be a lot more on this topic (I'll try to post a summary in a CSLA issue soon). All that said, I think for now we may want to hold off on the attribute approach for a bit. I'm not saying we should abandon it, but let's really think hard about all these different aspects before adding this in.

[kellyethridge]

I would assume the discovery process would happen only once per class for the lifetime of the app, possibly at the same time as adding business rules. So I think it would be a minimal one-time upfront cost.

[JasonBock]

@kellyethridge yes, so even if you had 10,000 business objects, each with 6 operations, the total cost would be about 0.16 seconds for the lifetime of that app, and 19.2 MB more allocations (hopefully my math was correct on that). But I'd like to explore a more over-arching approach to business object features, which might make this conversation mood regardless of performance.

[rockfordlhotka]

My prototype idea around registration is something like this:

  public readonly static IOperation FetchMethod = 
    RegisterOperation(Operations.Fetch, o => o.MyFetchMethod);
  private void MyFetchMethod()
  {
    // do stuff here
  }

[rockfordlhotka]

@JasonBock The thing about that 10k business class scenario is that CSLA loads metadata on-demand. So in real life that 0.16 seconds would only be paid if all 10k types were used. And even then it would be amortized over the period of time it took for the app to actually use all those types.

My primary performance worry is in serverless server-side scenarios where the appdomain/equivalent is torn town frequently. But in server-side scenarios it is far more common to use a very small number of types to respond to a single inbound request.

[JasonBock]

@rockfordlhotka I was saying that you would be loading all 10,000 through the lifetime of that application scenario. Which is somewhat unrealistic - I don't think anyone is loading that diverse of a set of BOs in an app :). I was pushing the limits a bit and even with that, the perf numbers are not....well, that bad. But again, there's a potentially larger design issue here at stake.

[JasonBock]

@rockfordlhotka what if you had two fetch operations that were overloaded?

[ajj7060]

I like this idea; it would overcome having to use the new keyword to hide some of the DP_XYZ (e.g., DataPortal_Execute) when the operation needs to be async, and we can properly name the methods with an Async suffix.

[tdyocum]

I really like this idea!

[rockfordlhotka]

Looking deeper into this issue, there are several options to choose from in terms of API design.

Option 1 - per-type attribute

  [DataPortal(Create = nameof(CreateAsync))]
  [Serializable]
  public class PersonEdit : BusinessBase<PersonEdit>
  {
    //...
    private async Task CreateAsync()
    { /* normal create code goes here */ }

    private async Task CreateAsync(int newId)
    { /* normal create code goes here */ }
  }

Advantages are performance (easy/fast reflection to find attribute), good method name refactoring protection, easy to implement in CSLA.

Disadvantages are that the attribute is not close to/attached to the methods in question, so readability isn't ideal, both method overloads (create/fetch) need to have the same name.

Option 2 - per-method attributes

  [Serializable]
  public class PersonEdit : BusinessBase<PersonEdit>
  {
    //...
    [DataPortal(DataPortalOperations.Create)]
    private async Task CreateAsync()
    { /* normal create code goes here */ }

    [DataPortal(DataPortalOperations.Create)]
    private async Task OtherCreateAsync(int newId)
    { /* normal create code goes here */ }
  }

Advantages are good method name refactoring protection, readability of code, the different create/fetch overloads could have different names.

Disadvantages this would require more and slower reflection, this is a lot harder to implement, (imo) it is bad that the two create/fetch overloads could have different names.

Option 3 - global config

  [Serializable]
  public class PersonEdit : BusinessBase<PersonEdit>
  {
    //...
    private async Task CreateAsync()
    { /* normal create code goes here */ }

    private async Task CreateAsync(int newId)
    { /* normal create code goes here */ }
  }

In this case the name of the "create" method would be defined via configuration. For example:

  new CslaConfiguration().DataPortal
    .Create("CreateAsync")
    .Fetch("FetchAync")

Advantages are performance (no reflection to find attribute), easiest to implement, (imo) no method name variation between business classes.

Disadvantages no method name refactoring protection, moves "magic method name" to your config instead of hard-coded, but still is magic, no increase in code readability over today, no method name variation between business classes.

Option 4 - static method registration

  public readonly static IOperation FetchMethod = 
    RegisterOperation(Operations.Fetch, o => o.MyFetchMethod);
  private void MyFetchMethod()
  {
    // do stuff here
  }

Advantages are performance (no reflection), easy to implement, consistent coding pattern with CSLA properties, method name refactoring, different overloads can have different names (create/fetch), method name variation between classes

Disadvantages people could put registration far from the methods in code and reduce readability, (imo) allows method name variation between classes

Option 5 - use method name provided by client calling code

  var obj = await DataPortal.CreateAsync(new SingleCriteria<PersonEdit, int>(5), nameof(CreateAsync)))

In this case the nameof(CreateAsync) expression provides the name of the server-side method the data portal will invoke.

Advantages are performance (no reflection), easy to implement, method name refactoring (in some cases), different CRUDE scenarios could have different method names

Disadvantages no method name refactoring (in some cases), increased dev complexity due to uncontrolled method names and (effective) overloads, data portal methods would need to be public b/c typically it is UI/client code calling the data portal

[rockfordlhotka]

@kellyethridge @JasonBock @ajj7060 @tdyocum - thoughts?

[GillesBer]

@rockfordlhotka / all, I believe that removing some 'magic' is the path to go.
Making our code more transparent is a good way to ease onboarding of newcomers.
Having several possible naming is not really OO oriented, imho.
So, I would personnaly push for your option 1 (class based attributes).

[JasonBock]

I would strongly go with Option 2. If I have a bunch of operations with overloads, doing it at the class level seems like kind of a PITA. The operation is a method, not something defined on the class. The speed thing is a possible issue - Option 1 only requires one reflection/attribute lookup operation (assuming caching), whereas Option 2 requires it per method.

Side note, I should re-run my perf tests I ran above for .NET Core 3.0. (Edit: I did that this morning, nothing looks substantially different from my original results).

I absolutely do not want Option 3. That would feel awful.

[tdyocum]

I'd like to see Option 2. I agree with Jason, I don't want option 3 at all.

[ghost]

What’s the change here once this is implemented? Going back to old code and updating the declaration or will old code still work?

[rockfordlhotka]

@fujiiface My plan is for old code to just work. I'll default the method names to the existing names so the data portal will keep working as-is unless it finds attributes/configuration to indicate different names.

[rockfordlhotka]

One other issue I've encountered in working on the code so far: the DataPortal_OnDataPortalInvoke and similar methods will probably retain their hard-coded values. In fact, they are normally invoked via the IDataPortalTarget interface, and so require no reflection or anything - and to make the method names dynamic would mean eliminating the use of that interface.

Do people have a strong opinion on making DataPortal_OnDataPortalInvoke, DataPortal_OnDataPortalInvokeComplete, and DataPortal_OnDataPortalException have dynamic names?

[rockfordlhotka]

One subtle thing to consider, looking forward to per-method dependency injection, is the need to differentiate between the create/fetch methods with and without a criteria parameter.

Your input is desired!

Option 1

  [Create]
  private void CreatePersonEditAsync()
  { }

  [CreateWithCriteria]
  private void CreatePersonEditAsync(int id)
  { }

Option 2

  [Create]
  private async Task CreatePersonEditAsync()
  { }

  [Create(WithCriteria = true)]
  private async Task CreatePersonEditAsync(int id)
  { }

or

  [Create(false)]
  private async Task CreatePersonEditAsync()
  { }

  [Create(true)]
  private async Task CreatePersonEditAsync(int id)
  { }

Now that I type it out, I think I have a strong preference for Option 2.

[ghost]

I like option 2. I feel like the attributes will look cleaner and conform to other attribute patterns I’ve seen.

[ajj7060]

Could the logic check to see if the received criteria matches the first, and then figure it out that way? Just thinking that if you assume the criteria will always be the first parameter, and try to match it up with the request, match just on that, and the any subsequent parameters would be resolved by the DI container.

I think I like [Create(WithCriteria = true)] because its more clear. Otherwise [Create(true)] its hard to tell just by looking at it what "true" does. (Unless you have R# i guess 😄

I have another question about this feature. Will the resolution of the dataportal methods be a logical server side thing? I ask because we build a Client and Server version of our business assembly and we use conditional compilation to exclude the data portal code. There's code on the server that we don't want to be available on the clients are its sensitive. Also, we deploy via clickonce to our customers but they don't like frequent updates, so having the code within the conditional compilation allows us to easily determine if a code fix can be done with only a server side update or if we will have to coordinate with our customers for a full release. So i'm hoping this will still be valid:

#if INCLUDE_DATA_PORTAL_CODE
[Create]
private void Create() {
 // Do create on server
}
#endif

[tdyocum]

I like Option 2. Its definitely cleaner to have the single attribute with the parameter as the modifier; versus two separate attributes. Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Christopher Fujii <notifications@github.com> Sent: Friday, May 31, 2019 6:03:53 PM To: MarimerLLC/csla Cc: Tim Yocum; Mention Subject: Re: [MarimerLLC/csla] Eliminate hardcoded DataPortal_XYZ method names (#1102) I like option 2. I feel like the attributes will look cleaner and conform to other attribute patterns I’ve seen. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#1102?email_source=notifications&email_token=ABWLYB2EPKQI4OWFJWYHKBDPYGOETA5CNFSM4HCM2FXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWWPJTY#issuecomment-497874127>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABWLYB2AELDK4UTB7M76P5DPYGOETANCNFSM4HCM2FXA>.

[Brannos1970]

I like Option 2 also. It seems to be much simpler.

[JasonBock]

I have to ask....why do we need different operation attributes depending on whether or not the method has arguments? @rockfordlhotka you know this far better than I do, but it does confuse me - I'm not seeing the reasoning behind this.

[rockfordlhotka]

@JasonBock I don't know, having not yet implemented the per-method DI. It would be good to have @keithdv's insight, as he did some prototyping right?

Just wondering how it would disambiguate this:

[Fetch]
private async Task FetchAsync(object criteria) {}

[Fetch]
private async Task FetchAsync(IDataLayer dal) {}

With the first one having a type object it may be impossible to match any passed criteria by parameter type, and yet that's the overload to call.

Update: I did some experimentation and I suspect the answer is fairly simple.

If the first parameter of your create/fetch method is not an interface (pType.IsInterface == false) then I should be able to assume it is your criteria.

That'd allow me to know that the first of the two methods in this example is the one that accepts criteria, because its parameter type isn't an interface and therefore can't be resolved by DI.

Does that seem reasonable? It does mean nobody could pass their criteria as an interface type, but I don't think anyone does that anyway? I hope?

[ajj7060]

@rockfordlhotka I've written code that takes an interface for the criteria class and its public (code in use today). Its something like this

public interface IOrderSearchCriteria : ITrackStatus { }

public sealed class OrderSearchCriteria : BusinessBase<OrderSearchCriteria>, IOrderSearchCriteria  {
  // /csla properties; this class is bound a a search form and there are actual business rules so you can't combine certain search properties or leave it completely unset which would return everything
}

public sealed class OrderSearchList : ReadOnlyList<OrderSearchList, IOrderSearchInfo> {
    public static OrderSearchList FindOrders(IOrderSearchCriteria criteria) {
        if (!criteria.IsValid) { throw new ArgumentException(); }
        return DataPortal.Fetch<OrderSearchList>(criteria);
    }
}

Of course the main reason for this is so that we can unit test the OrderSearchList class without worrying about the behaviors implemented in concrete OrderSearchCriteria class.

[SergeyBarskiy]

DataPortal knows the criteria type though or a business object type for the update. It would be better to match on that I think,

[JasonBock]

Are we assuming only one object can come in as a criteria? Is there any reason why we can't do this:

[Fetch]
public async Task MyFetchAsync(int id, string name) { ... }

Or are we always forced to have one, and only one, object? But that can be overloaded?

I guess what I don't understand is that C# is able to disambiguate between valid overloads. So, why can't CSLA? Or is this because the call is remote and you have that params support thingee? :)

[SergeyBarskiy]

@JasonBock
I believe DataPortal.Fetch has only support for a single criteria object for the root fetch. Child fetch supports unlimited parameters via params object[]. This will be harder if you want to support also method dependency injection, kind of forced by the concept of data portal. But I do agree, I also do not see a reason for paramterized fetch, but maybe it would be good to have Fetch and ChildFetch attributes ?

[rockfordlhotka]

@SergeyBarskiy you are right - we will need ChildFetch or FetchChild attributes as well.

I sort of lean toward CreateChild and FetchChild naming because I think that'll make intellisense more useful, but am open to arguments for putting Child first.

[rockfordlhotka]

I wrote up the algorithms for criteria-provided an no criteria for method resolution in #787 (comment). I'd appreciate your thoughts on the algorithm to see if I got it right (@SergeyBarskiy @JasonBock @dazinator and anyone else).

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.