Consider requiring data portal operation methods to be public

[rockfordlhotka]

@JasonBock and I have been chatting about the possibility of making the data portal operation methods (domain class methods with data portal attributes like Fetch) public instead of private.

These methods have been private by convention (and recently by analyzer) forever, because they must not be invoked directly. They are only to be invoked by the data portal.

However, there are a couple drawbacks to them being non-public.

1. Standard code analyzers flag these methods because they aren't invoked anywhere
2. There's a slight performance cost to using reflection to find/invoke a private member vs a public member on a class
3. There is the possibility of .NET having a linker optimization that just eliminates these methods from the binary output, because they aren't invoked explicitly and so look like dead code (this just happened to the ObjectAuthorizationRules private static method that sets up per-type rules)

Changing the method would break everyone's existing code, though the fix would be as simple as changing the scope on all the methods - and that's something an analyzer could essentially automate.

If the methods were public then people could call them directly, and that would be BAD. However, an analyzer could make such explicit calls an error, preventing that from happening.

Steps to make this change:

• Change the existing analyzer that ensures the methods are private to ensure they are public
• Add a new analyzer to prevent any code from explicitly invoking these methods
• Change the data portal to expect these methods to be public

[TheCakeMonster]

I would personally feel more comfortable if this change was not made. To me it feels like we are going further down the road of breaking the encapsulation that CSLA has done so well in the past - another step on from the requirement for constructors to be public, which I also continue to feel uncomfortable about.

Analyzers to tell the developer off would certainly help with the day to day reality of writing the code, but I'm not sure this is the same as making the framework more easy to understand from a theoretical perspective. An analyzer telling someone off is also quite late in the day for anyone who has read the code as part of making a change, and made an invalid assumption about what they can and cannot do - in other words, they have made design decisions in advance of making a code change. Being unable to make the change they thought they could make (the way they thought they could make it) could mentally derail someone, and cause delivery delays, and frustrated developers.

From a user experience perspective, it is better not to allow someone to do something, than allow them to do it and then tell them off for doing so. Analyzers feel like the latter.

Intellisense and Object Browser should help people understand the public interface of the objects they work with, and that applies equally to CSLA business objects. Whilst we could make them public and then add attributes to hide the methods from Intellisense, it feels better to me to just use scope the way it was intended to be used.

There are an alternate solutions to the linker/reflection problem identified. The most obvious is to attribute classes with the DynamicallyAccessedMembersAttribute. You can only do this in .NET 5 and above, which is a shame, but it is probably something that should be done to CSLA classes. I say that because CSLA specialises in minimising the addition of code that isn't needed to deliver business functionality. We don't add things to CSLA business classes that we don't need on a day to day basis, so applying trimming/tree shaking to them seems irrelevant to me. Indeed, it's tempting to say that this should be applied at an assembly level, although I don't know of a concrete mechanism to do that.

For reference, here is an issue from the dotnet repo in which they discuss, and then reject, making the DynamicallyAccessedMembersAttribute available to .NET Standard assemblies.
dotnet/runtime#36656

That decision is a great shame, I have to say. Microsoft's continuing desire to ignore legacy code, such as people needing to span .NET Framework and .NET (Core), is disappointing. I wish that they would stop saying that .NET 5 is the new .NET Standard, because it isn't. It doesn't cater for .NET Framework, and therefore all of the systems written in the 20 years before .NET Core.

[TheCakeMonster]

I should be clear that when I talk about adding the DynamicallyAccessedMembersAttribute, I'm talking about recommending that developers who use CSLA add it to their business classes. Their business classes are built to achieve a specific purpose; they don't add a lot of code that won't be needed. As a result, the benefit of their classes being considered for trimming by the linker is probably small, IMO.

[rockfordlhotka]

That is my primary concern: linker optimization that eliminates these methods. In all other regards I agree that keeping them non-public is preferable.

[TheCakeMonster]

I've raised an issue on the .NET repo about supporting the necessary attribute on .NET Standard 2.0, therefore being able to add the necessary support to CSLA if needed. There is text in the issue I linked above that hints at a way to provide this support despite wanting to support .NET Standard, but I don't yet understand how it would work.

dotnet/runtime#56612

Attributing our way out of linker issues feels like a reasonable next step, even if it proves to be a failed experiment.

[TheCakeMonster]

I spent a day trying to use DynamicallyAccessedMembersAttribute, and failing to get it to quite do what I was expecting. I imagined that applying this attribute to a class would stop any trimming, but it doesn't seem to do that - at least not with .NET 6 preview 6 installed.

I've not given up, but I have had to move back to other things in the short term. My next step is to open another issue on the dotnet/runtime repo, but I need to tidy up/simplify my sample code before I expose it as a reproduction of the behaviour I am seeing.

I remain mightily impressed by - and eternally grateful to - those folks who find time to do lots of work on open source projects. I seem to be poor at doing more than one project at a time, so at the moment I'm not managing to contribute quite as much as I intend to whilst I finish off the application I am working on. That doesn't mean I won't, it just means I haven't - yet. Past performance is no guarantee of future success ;-)

[TheCakeMonster]

Issue on DynamicallyAccessedMembersAttribute raised on the dotnet repo. This is important to us as it is this attribute that I am suggesting we use to avoid trimming of data access methods in CSLA types.

dotnet/runtime#57140

Repro of the attribute failing to perform as I would hope, for it to be useful to us, available at:

https://github.com/TheCakeMonster/Examples/tree/master/Issues/UnexpectedTrimming

This sample also demonstrates the recommendation of how to get code using attribute to compile into a .NET Standard library, which we will want to make use of for backwards compatibility and simplicity.

[rockfordlhotka]

@TheCakeMonster I think the trimmer issue is discussed in a different backlog issue: #1794

[swegele]

That is my primary concern: linker optimization that eliminates these methods. In all other regards I agree that keeping them non-public is preferable.

Just a quick word of agreement with and appreciation for the way @TheCakeMonster summarized the concern that has been with him since making CTORs public...and concern about further drifting that way.

Allow me to wish you good luck in figuring out the attribute trick for dealing with undesired trimming. Thank you for all the wonderful contributions...for this breadcrumb path so often helping me navigate :-)

[rockfordlhotka]

I am going to close this issue.

I really don't want to make the data portal operation methods public. People already struggle with the idea of relying only on the data portal for persistence and server communication, and making these methods public would lure people into invoking them directly.

The better solution is to focus in #1794 to block trimming of the non-public methods. If that turns out not to work we can revisit this entire idea - but I think it is a really bad idea overall.

[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.