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 - https://github.com/dotnet/coreclr/pull/21832
  • Optimization: Reduce many array allocations in GetCustomAtttributes()'s inherited path - https://github.com/dotnet/coreclr/pull/20779
  • Optimization: Further reduce allocations in GetCustomAtttributes() paths - https://github.com/dotnet/coreclr/pull/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?

[ghost]

I haven't explicitly used DataPortal_OnDataPortalInvoke, DataPortal_OnDataPortalInvokeComplete, and DataPortal_OnDataPortalException so I'm ok with it.

[SergeyBarskiy]

What about just DataPorta.Create(new SingleCriteria<PersonEdit, int>(5), nameof(CreateAsync)))?
Side note: although reflection performance overhead maybe manageable, attribute operation is one of the slowest in reflection. Also, I think it is cached after the first call I believe (?), so creating loop tests may not produce correct results. From personal experience I can tell you that if you take a memory dump of a sizable CSLA app, you will see tens of thousands of CSLA objects that are statically held in memory. Even property info objects aren't free. One could argue premature optimization is a bad thing, but if performance overhead is coded into api, it will be super hard to fix in the future. I have personally slowly came to conclusion that memory and CPU overhead should not be disregarded. If there is a way to avoid that from day one, that would be a good thing. I think this is an opportunity to get rid of magic strings but maybe also improve performance by a little bit? My 2 cents...

[JasonBock]

If code currently does this:

private void DataPortal_Fetch() { … }

We (and by "we" I mean "me" :) ) could create an analyzer that looks for methods with well-known "legacy" DataPortal names, and provide a fix to add the attribute, which could be done solution-wide.

[JasonBock]

@SergeyBarskiy

What about just DataPorta.Create(new SingleCriteria<PersonEdit, int>(5), nameof(CreateAsync)))?

So you're putting the onus on the caller to provide the right name? Am I understanding this right? If so, what if the method is private? This doesn't seem like the right choice.

Also, I think it is cached after the first call I believe (?), so creating loop tests may not produce correct results.

Are you talking about within the Reflection API, or CSLA itself?

From personal experience I can tell you that if you take a memory dump of a sizable CSLA app, you will see tens of thousands of CSLA objects that are statically held in memory. Even property info objects aren't free.

This seems like an issue, but it's not clear if this is a universal issue that every user of CSLA will encounter, or it's specific to an application usage.

I think this is an opportunity to get rid of magic strings but maybe also improve performance by a little bit?

It is a way to get rid of opinionated, magic names. From a perf perspective, I did a bunch of analysis on using attributes (which is included in this issue), and yes, it is slower, but that slowness is arguably neglegible.

[JasonBock]

@rockfordlhotka you included three options above, what about the operation registration idea? Have you abandoned it, or is that still on the table?

[rockfordlhotka]

That's still on the table - I rather forgot about it... Can you refresh my memory with a code example?

[JasonBock]

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

That's what you posted above :)

[rockfordlhotka]

@SergeyBarskiy the problem with that approach is that the code you show is on the client, but the data portal is invoking that method on the server. Challenges:

1. Sometimes people use compiler directives to prevent the server-side code from even existing on the client, in which case the only way for the client to indicate the method would be via string literal
2. In any case we'd need to convert the method name to a string so it could be passed via the data portal and used to dynamically call the method on the server

[rockfordlhotka]

@JasonBock oh - doh! 😳

And it is probably the best of all the options too...

I CREATED OPTION 4 ABOVE with the RegisterOperation concept.

[ajj7060]

Yikes, lots of discussion such a short time! I think I like option #2 the best, followed by #4. I'll have to read the thread in more detail later though and maybe give some reasons why (if that is helpful).

[JasonBock]

@rockfordlhotka

And it is probably the best of all the options too...

I may include that into my perf analysis, just to see if it matters. Though what really matters in that case is what RegisterOperation() would actually do, and how it was defined.

[ajj7060]

IMO, I think I still like option 2 the best, the attributes. The attribute would only require a very tiny amount of code, and allows the flexibility to name the methods whatever you want, which is what I like most about this feature. We can follow naming guidelines (eg., private void Create(int), private Task CreateAsync(Criteria) in the same class or not).

I can live with option 4 as well, but with the discussion to inject dependencies into the method, I think it gets a bit messy if we would need to do this:

public static readonly IOperation CreateOperation = RegisterOperation(Operations.Create, x => x.Create(null, null, null, null, null, null));
private void Create(object criteria, object dep1, object dbConnection, object somethingParser, object messenger, object complexCalculator) { }

The naming flexibility is my main concern though.

[SergeyBarskiy]

I'll try to answer some questions.

1. Should not matter if the method is private or does not exist in the scope (aka client vs server). You can always just pass in the string with the name instead. Though method should be public for testing anyway,
2. There are a lot of CSLA objects in memory, such as reflection cache, property info objects and more. Probably reflection API is there as well. To some extent every app will have that, but a large number of static variables makes it worse.
3. Yep, I agree that overhead of attributes is negligible in terms of overall app performance overhead. Then again, if there is an opportunity to take no overhead, that is better, again subjectively so.

[JasonBock]

@SergeyBarskiy

1) I guess I don't understand where you're proposing to call DataPortal.Create() and passing in the method name as a string. If it's on the client, and I'm consuming the BO, then I don't know of a clean way to get the method name other than putting it in as a hard-coded string, which is fragile. Again, maybe I'm getting what you're proposing here, but if I am understanding it correctly, then I'm definitely against that. And I don't like have operations public at all. This can easily lead code astray as an operation can be called in a matter that doesn't fit within the expected CSLA lifecycle. Having something available for testing purposes is a sign that there are compromises being made just to satify test cases.

2) I think @rockfordlhotka is trying to eliminate more static variables in CSLA over time. I agree, those are prone to leaking memory (or holding on to things for too long in an app), but it's not clear to me what exactly is the problem here, and if it is some kind of bug in CSLA.

3) I personally want something faster, even if we're talking micro- or nano-seconds. That said, this does have to be weighed with other factors, like having a "safe" way to mark operations without having one character missing and not having CSLA work correctly with that operation.

[rockfordlhotka]

There's no doubt that calling arbitrary methods on the server would be pretty cool. It is something I played with back in 2001 or so - and ultimately rejected it because I thought it would open the door to a whole lot of potential complexity. Very cool, but conceptually a bit complex (not technically so much).

Pass an arbitrary object graph to an arbitrary method and get back an arbitrary result.

That'd be fun to do some day perhaps, but right now my focus remains on create/fetch/update/delete/execute within the context of the CSLA data portal.

So sticking within the context of CRUDE, my primary concern is different - we grant the calling code (often the UI) with the power (and therefore responsibility and required knowledge) of server-side operations. In other words, my concern is that having the UI need to know and provide the name of the server-side method reduces encapsulation and abstraction. I don't think that's a good thing.

[SergeyBarskiy]

@JasonBock I believe you follow what I am saying. As far as static variable go, static prop bla = register property() will leave in memory forever by definition. The same applies to proposed register method. On top o that there are static variables in CSLA reflection class. Maybe minor overhead, but nothing a developer can do about it. When it comes to hosting costs, memory footprint does matter IMHO. Again, in a small-ish app probably a few megs, large app maybe larger. Not sure about cost differences either. By comparison, parts of EF are also cached in memory, and DbContext of 100 tables will likely be much larger in terms of memory footprint than all of CSLA things combined. Just my perspective though. Maybe jaded somewhat, as we are in a process of trying to reduce hosting costs of a large app.

Side topic, but I think is relevant. What about going away from reflection altogether and relying on something like Fody https://github.com/Fody? You can potentially remove all overhead and enable coding scenarios that look something like plain vanilla classes and turned into CSLA classes by weaver.

public string Blah { get ; set; }

[DataPortal(Fetch....)] public Task GetchMyStuffAsync()

Then invest into a weaver and reduce overhead tremendously, along side with elimination of property registration and method registration. The attribute is only relevant at compile time too. Maybe have to coerce :-) Magenic into becoming a sponsor.

[rockfordlhotka]

For the root data portal the perf per call (probably) isn't a big deal, and an argument could be made that we don't need to use expression trees and cache the results to avoid reflection.

For the child data portal this is absolutely not true. The pre-optimization performance vs the current performance is/was a really big deal when loading collections.

[JasonBock]

@SergeyBarskiy, trying to add in something like Fody to CSLA feels like something CSLA should not have a dependency on. Plus, mentioning adding dependencies to other packages in CSLA tends to make @rockfordlhotka twitch a bit :)

[SergeyBarskiy]

I do not recall every seeing @rockfordlhotka twitch. I would like to see that. I assume there are already dependencies in place on every possible .NET Core assembly, right? Potentially Fody would completely eliminate the need for reflection...

[rockfordlhotka]

I've learned to twitch on any dependency not in the .NET "box" so to speak. Or OSS projects with the longevity and track record of something like CSLA.

I even got screwed by nunit at one point - my bar for taking dependencies is terribly high...

[rockfordlhotka]

Back to our list of options, which now includes Option 5 as per @SergeyBarskiy.

I find that most people are learning toward option 2 (per-method attributes), with some liking option 4 (registeroperation) as well.

[rockfordlhotka]

At this point I'm strongly leaning toward Option 2 - a per-method attribute.

Attributes: Create, Fetch, Insert, Update, Delete, DeleteSelf

Speak now or forever hold your peace 🗣

[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 iOShttps://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 GitHubhttps://github.com/MarimerLLC/csla/issues/1102?email_source=notifications&email_token=ABWLYB2EPKQI4OWFJWYHKBDPYGOETA5CNFSM4HCM2FXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWWPJTY#issuecomment-497874127, or mute the threadhttps://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,