Skip to content

Possible interface proxy unnecessary initialization #2271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bahusoid opened this issue Nov 12, 2019 · 5 comments
Open

Possible interface proxy unnecessary initialization #2271

bahusoid opened this issue Nov 12, 2019 · 5 comments

Comments

@bahusoid
Copy link
Member

bahusoid commented Nov 12, 2019

If proxy interface IProxy implements multiple interfaces with identifier property:

interface IReadOnlyId
{
	int Id { get; }
}

interface IId
{
	int Id { get; set; }
}

//IProxy contains both IReadOnlyId.Id and IId.Id getters. It's ambiguous but valid code
interface IProxy : IId,  IReadOnlyId
{
}

public class Entity : IProxy
{
	public virtual int Id { get; set; }
}

Currently generated proxy initializes proxy IProxy on IReadOnlyId.Id access:

IReadOnlyId entity = session.Load<IProxy>(id);
Console.WriteLine(entity.Id); // <- entity will be initialized on entity.Id access

It should not trigger proxy initialization.

Possible solution: implement class for interface proxy members implicitly.

@fredericDelaporte
Copy link
Member

fredericDelaporte commented Nov 12, 2019

When building an interface proxy, NHibernate has currently no knowledge of the concrete class which will implement it under the hood (as a delegated state). So it cannot analyze it to determine which interface members should be implicitly implemented or not.

Implementing them all implicitly could lead again to duplicated methods in the proxy, and could be invalid depending on whether the concrete class explicitly implement them or not.

With an interface proxy, only the declared proxy interface id property can be considered as being the id currently.

Changing this would require to transmit that concrete class to the proxy builder, which is not currently done. It may be not easy to do. And then the proxy implementation should be adjusted to generate virtual counterparts to implicitly implemented methods, instead of overrides (excepted for object instance methods like GetHashCode and Equals which would have to stay as overrides).

@bahusoid
Copy link
Member Author

With an interface proxy, only the declared proxy interface id property can be considered as being the id currently.

I'm not sure I understand what it means. Are you saying if actual mapped entity implements both interfaces implicitly - it should work? Maybe my test is simply wrong (it's with subclasses and base class doesn't implement both interfaces) - I will double check tomorrow. But with simple example which is illustrated in this issue - should it work properly theoretically?

@fredericDelaporte
Copy link
Member

fredericDelaporte commented Nov 12, 2019

Should, maybe, would, no.

The technical current state of proxy generation does not allow this to work in a reliable way.
Either it (again) would have to do some weak assumption based on naming, or it does not support it.

With an interface proxy, only the declared proxy interface id property can be considered as being the id currently.

I was meaning only what matches the mapping id declaration in the proxy interface can currently be recognized without doubt as being the id. But now I realize the ambiguity: there are two candidate properties in IProxy matching the mapped id.
Which ones NHibernate ends-up picking as "the" id? It seems possible this case may generate failures, depending on the actual logic NHibernate uses to identify which one is "the" id property. Maybe it may pick-up the read-only one and be stuck for generating its setter. (This may occur by changing interface declaration order, or alphabetic sort, ... (Sometimes implementation order depends on naming, though this is unspecified behavior.))

Due to xml mapping semantic, it seems now clear to me this should be supported. But detecting it and supporting it from IProxy inspection may require some wicked code.
And if some wicked developer then decide to explicitly implement one of the interfaces to yield something else, we will have to explain the mapping semantic does not allow to support such a case.

@bahusoid
Copy link
Member Author

I simplified test to make it as simple as described in this issue - and issue still occurs. So I confirm it's valid issue.

But detecting it and supporting it from IProxy inspection may require some wicked code.

It seems to me that interface proxy members can be implemented implicitly (and simply skip duplicate members). This logic should be applied strictly when proxy is interface.

And if some wicked developer then decide to explicitly implement one of the interfaces to yield something else, we will have to explain the mapping semantic does not allow to support such a case.

I don't get this point. Isn't it already the case for interface proxy:
Explicit interface implementation for entity can have different meaning from the one implemented by generated interface proxy.

@fredericDelaporte
Copy link
Member

I don't get this point. Isn't it already the case for interface proxy:
Explicit interface implementation for entity can have different meaning from the one implemented by generated interface proxy.

Yes, but a proxy implementing interfaces implicitly will then use the same method to implement both. And by the way, how do we decide through which interface the common implicit implementation should call the corresponding method on its internal state?

There will be a few hard troubles to tackle for this issue.

hazzik pushed a commit that referenced this issue May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants