Skip to content
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

Ability to specify proxy generator or invalidate it #925

Open
bclothier opened this issue Sep 7, 2019 · 6 comments
Open

Ability to specify proxy generator or invalidate it #925

bclothier opened this issue Sep 7, 2019 · 6 comments

Comments

@bclothier
Copy link

bclothier commented Sep 7, 2019

This was originally opened here.

Basically, the issue is that Moq uses a single ProxyGenerator, which is fine for 99% of the scenarios. However, I have a scenario where I'm dealing with types that get created at runtime and consequently, can cause collisions with the proxy type cache because the type can change.

The ideal solution would be to be able to use a MockRepository and provide a proxy generator or at least specify that it should use its own proxy generator, separate from the rest of proxy generator, so that any mocks based on those volatile types won't get cached for very long and can be easily invalidated.

Back this issue
Back this issue

@stakx
Copy link
Contributor

stakx commented Sep 8, 2019

We'll have to be very careful not to create a leaky abstraction.

  • Allowing users to state that they want a repository to use a separate proxy generator only makes sense if Moq has an explanation for why one would ever want to do that. Right now, the explanation would be, "because that will cause the repository to have its own proxy type cache". That explanation would very much be tied to DynamicProxy, because...

  • Any notion of "proxy type cache" isn't valid in the general case as there could be proxy factory implementations that don't cache the generated types.

  • Any notion of "invalidate the proxy generator" also doesn't make much sense to me in the general case... what exactly does that mean?

The above points lead me to believe that we need to let client code choose a proxy factory. That way, we can expose the DynamicProxy implementation and document its exact semantics, and keep them separate from the more general proxy factory concept.

I also mentioned in the Gitter chat that Moq v4 isn't quite ready to fully expose its proxy factory abstraction. That is mainly because it, as well as other parts of Moq's design, have been heavily influenced by the capabilities of DynamicProxy, e.g. the fact that you cannot mock static or sealed types, and that you cannot set up non-virtual methods. If we open up Moq to other proxy factory implementations (that may be more powerful, e.g. those based on the CLR's profiler API), we'd have to rewrite large parts of Moq so it is able to deal with statics, non-virtuals, etc. For now, that is out of the question due to the very large amount of work involved.

All of the above leads me to the following proposal:

  1. We expose ProxyFactory as an abstract base class which (for now) has no public members, and a constructor that is internal or private protected. This keeps the set of concrete implementations under Moq's control. Once Moq is ready to allow custom implementations, we can open up that type for subclassing.

  2. We expose a single concrete implementation for ProxyFactory, DynamicProxyProxyFactory, and document its behavior (e.g. its limitations, and the fact that it uses a proxy type cache).

  3. We add a read-write property ProxyFactory to MockRepository that by default is set to a default (global) instance of DynamicProxyProxyFactory, but can be set to a new instance. (This is what would enable you to use a fresh ProxyGenerator with an empty proxy type cache.)

  4. We possibly add a static read-write property Default to ProxyFactory that is likewise set to the same default instance of DynamicProxyProxyFactory. This factory gets used for all mocks that are created globally, i.e. not through a MockRepository.

  5. We might have to add an internal ProxyFactory property to Mock such that MockDefaultValueProvider can discover and use the same proxy factory when auto-mocking properties.

Will the last bit be problematic in your scenario? Say you have a mock with mock.DefaultValue == DefaultValue.Mock. This was created from proxy factory instance A. Now you query mock.SomeProperty which is of a mockable type, and MockDefaultValueProvider auto-mocks the value, i.e. creates another mock, also from proxy factory instance A. We now have two mocks created from the same factory. Is it possible that any of those survives a VBA code execution? If so, your problem isn't solved by the above.

Any thoughts or opinions on the above?

@bclothier
Copy link
Author

bclothier commented Sep 8, 2019

Yes, I agree with your thoughts regarding avoiding leaking the abstraction. Obviously you see that I'm too focused on solving the issue at hand and not taking a step back toward the overall picture. With your description of what needs to be done, I think that custom proxy factory is sufficient without breaking the abstraction.

RE: item 4 That falls into a category "nice-to-have" because in my mind, if I'm customizing the proxy factory, I should be specific about where I want to use it. In my use case, at least, not all types are volatile --- for example, I might obtain a type from Excel interop library. That type won't change between VBA code executions. OTOH, the type from VBA is volatile, and I have no guarantee that it's still the same type next execution.

That said, I'm not sure that is relevant to Moq's considerations but just so you are aware - I also have to maintain a type cache but for different reasons. To avoid runtime errors such as invalid cast or type mismatch, I have to make sure that there is only one instance of type used, and that should be the same between runs in the case of referenced libraries because referenced libraries are persistent and needs to be comparable. I could choose to reset the cache between runs but that would be wasteful because referenced libraries' types won't change anyway but I need to ensure we don't accidentally create new types for same representation. Whether that paragraph is applicable to Moq's ProxyFactory, I'm not sure, since I would be the one who's managing the concern, not Moq, I think.

RE: item 5 -- I actually learnt about the property now that you mentioned it. Doing some reading on it, yes I think that as long the MockDefaultValueProvider uses the same proxy factory, it will be fine, since we can ensure that none of the types created via either routes survive to the next execution run.

All in all, I like the idea of being able to request a new proxy factory in the mock repository and that would make my life easier.

@stakx
Copy link
Contributor

stakx commented Sep 8, 2019

@bclothier, some quick questions:

  • It appears it's possible for your add-in to be notified when VBA execution starts. Correct?

  • If so, is the use of AppDomains an option at all? If so you'd have a way of unloading previously generated dynamic assemblies and start type generation from scratch, without us having to change Moq.

  • If so, but you can't use AppDomains, that would still seem like an appropriate time to set up a new proxy factory (once Moq supports that).

  • Re: my point (5) above, I was worried more about whether mock objects can survive a single VBA code execution session into the next... not just the proxy types associated with mock objects. Can you confirm that no mock objects are kept around when VBA code execution stops?

@bclothier
Copy link
Author

Hadn’t considered using AppDomain. I need to verify whether I can create temporary AppDomain and whether I can control what goes in the AppDomain. If bith hold, then, yes I can create one in response to the start of VBA execution.

No, mocks should not survive the execution run. In theory, one could create a public field and store a mock in the field. That would survive execution runs, but frankly I can’t see a good reason to persist mocks in this manner and I won’t support that scenario. Mocks are meant to be transient anyway.

@Znurre
Copy link

Znurre commented Jan 25, 2022

I have created a PR #1227 with some initial changes meant to initiate a discussion about implementing this.

Please have a look :)

Copy link

Due to lack of recent activity, this issue has been labeled as 'stale'.
It will be closed if no further activity occurs within 30 more days.
Any new comment will remove the label.

@github-actions github-actions bot added the stale label Aug 24, 2024
@github-actions github-actions bot removed the stale label Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants