MarimerLLC / csla

A home for your business logic in any .NET application.
https://cslanet.com
MIT License
1.26k stars 402 forks source link

Dataportal operations of base classes are invoked #4090

Closed StefanOssendorf closed 2 months ago

StefanOssendorf commented 3 months ago

(as discussed here https://github.com/MarimerLLC/csla/discussions/4074)

Currently CSLA will invoke data portal methods (Create, Fetch, ...) defined by a base class. This is unexpected and possibly error prone. Because of this that functionality must be removed from the current data portal method invoker/finder.

Examples which currently work but must not be supported after finishing this task:

public abstract class FooBase : BusinessBase<FooBase> {
  [Create]
  private void CreateBase() {
  }
}

public class Foo : FooBase {
}

public class BarBase : BusinessBase<BarBase> {
  [Create]
  private void CreateBarBase() {
  }
}

public class Bar : BarBase {
}

Version and Platform CSLA version: 8.x OS: Windows Platform: WPF, Blazor, Console

Notes to add to the upgrade doc

# CSLA data portal method resolution of base classes removed
Up to this version csla would resolve data portal operations of a base class when defined but not "overriden" by the actual instantiated type. That means in the future all (non abstract) concrete types have to define the operations.

Here are some examples with old the old way and the new one.
```csharp
// Old possible way which will not work anymore
public abstract class FooBase : BusinessBase<FooBase> {
  [Create]
  private void CreateBase() { }
}

public class Foo : FooBase { }

// New
public abstract class FooBase : BusinessBase<FooBase> {
}

public class Foo : FooBase {
  [Create]
  private void Create() { }
}
\```

This also applies to non abstract inheritance relations.
```csharp

// Old way which will not work anymore
public class Foo : BusinessBase<Foo> {
  [Create]
  private void Create() { }
}

public class Foo2 : Foo {
}

// New
public class Foo : BusinessBase<Foo> {
  [Create]
  private void Create() { }
}

public class Foo2 : Foo {
  [Create]
  private void Create() { }
}
\```

If you want to have common logic in a base class you now have to make it available as a `protected` method which will be used by all inheritors.

```csharp
public abstract class FooBase : BusinessBase<FooBase> {
  protected void CommonCreate() { }
}

public class Foo : FooBase {
  [Create]
  private void Create() {
    CommonCreate();
  }
}
\```
luizfbicalho commented 2 months ago

Just to be clear here

public abstract class FooBase : BusinessBase<FooBase> {
  [Create]
  private void CreateBase() {
  }
}

public class Foo : FooBase {
}

this can't work anymore

public abstract class FooBase : BusinessBase<FooBase> {
  [Create]
  protected void CreateBase() {
  }
}

public class Foo : FooBase {
}

this can't work anymore


public abstract class FooBase : BusinessBase<FooBase> {
  [Create]
  public void CreateBase() {
  }
}

public class Foo : FooBase {
}

this should work right?

public abstract class FooBase : BusinessBase<FooBase> {
  [Create]
  protected virtual void CreateBase() {
  }
}

public class Foo : FooBase {
}

this should work or just with the override?

rockfordlhotka commented 2 months ago

Thank you for the analysis.

I think that the goal is to ignore non-virtual methods in base types.

Your question about virtual methods is interesting, and I personally think I'd expect to be able to override a base method and have it still be invoked by the data portal. But only if I override it. And in that case, I can reapply the attribute easily enough.

So in the end, I think we should only honor attributes in the concrete type, not in any base type.

luizfbicalho commented 2 months ago

Can you assign that to me?

michaelcsikos commented 2 months ago

I would expect protected and protected virtual to still work if they are decorated with any of the DP operation attributes.

In a base class we often do things like:

[Create]
[CreateChild]
protected virtual void Create()
{
    Id = Guid.NewGuid();

    BusinessRules.CheckRules();
}

I shouldn't have to override that method if there's nothing more to do.

michaelcsikos commented 2 months ago

Also, with the way it works now, if I see that a DP method is an override I know that I don't have repeat the DP operation attribute.

michaelcsikos commented 2 months ago

If a base class requires a method to be overridden it should be abstract. If it's virtual, there's a default implementation which should still work.

michaelcsikos commented 2 months ago

Sorry to pepper this issue with comments, but I really don't want to have to add this to thousands of classes:

[Create]
[CreateChild]
protected override void Create()
{
    base.Create();
}

And if the attributes are only honoured in the concrete type, there would be no other solution, right?

luizfbicalho commented 2 months ago

I think that @michaelcsikos and @rockfordlhotka should make a table like in the https://github.com/MarimerLLC/csla/issues/4090#issuecomment-2257169828

rockfordlhotka commented 2 months ago

I think what @michaelcsikos is suggesting - and I agree - is this?

base concrete behavior
private none base attribute/method ignored
public none invalid code
protected none base attribute honored; base method invoked
protected virtual none base attribute honored; base method invoked
protected virtual override base attribute honored; concrete method invoked
protected abstract override base attribute honored; concrete method invoked
luizfbicalho commented 2 months ago

invalid code

what do you mean by invalid code?

rockfordlhotka commented 2 months ago

invalid code

what do you mean by invalid code?

Data portal operation methods are not allowed to be public.

StefanOssendorf commented 2 months ago

Sounds good to me, too.

luizfbicalho commented 2 months ago

invalid code

what do you mean by invalid code?

Data portal operation methods are not allowed to be public.

the analyzer checks for this right? should this invocation throw an exception in this case?

rockfordlhotka commented 2 months ago

invalid code

what do you mean by invalid code?

Data portal operation methods are not allowed to be public.

the analyzer checks for this right? should this invocation throw an exception in this case?

There's already an analyzer, and I'm pretty sure the reflection code excludes public methods, and so would never find a public method.

StefanOssendorf commented 2 months ago
How should this scenario be defined? base concrete behavior
protected private new ??

Code example (I'll omit the csla classes etc to keep it compact)

public class Foo {
  [Fetch]
  protected void Method1() {
  }
}

public class Bar : Foo {
  [Fetch]
  private new void Method1() {
  }
}
luizfbicalho commented 2 months ago

How should this scenario be defined?

I think that the private new will be used as it's not related to the protected method, it's not an override.

rockfordlhotka commented 2 months ago

How should this scenario be defined?

I think that the private new will be used as it's not related to the protected method, it's not an override.

That is what I would expect.