Skip to content

Other context type support #3

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
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

mashbrno
Copy link

I've finished support for other contexts. Extracted the functionality to extension methods so it can be easily implemented with any type of Context. Unfortunately without tweaks in core Spring.Net I was unable to do it nicely, and custom Context type is required. That is why there are new FluentXmlApplicationContext, FluentWeApplicationContext.

Next I'm trying to work on support for AspNetCore. So please discuss first if you want to remove some of pulled classes.

Copy link
Owner

@Suremaker Suremaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @mashbrno .

I left some comments about the code. Please let me know what do you think.

Generally, please:

  • Adjust the projects and solution to make it compiling and producing packages properly,
  • Please add tests to the code you have introduced. If needed, feel free to create Spring.FluentContext.Web.UnitTests similarly to Spring.FluentContext.UnitTests
  • Please do add the example usages of the new contexts. It will help understanding the reason of having them and the usage patterns. ATM I have no idea on what is the purpose of FluentXmlApplicationContext. I am also not sure how are you going to use the FluentWebApplicationContext and how the registration code would look like. The sample usage will definitiely help here.

Thanks!


namespace Spring.FluentContext.Web
{
public class FluentWebApplicationContext : WebApplicationContext, IFluentConfigurableApplicationContext
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests to this class, especially for OnPreRefresh that has own logic that should be covered by tests proving it works as expected.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally it would be best if I introduce custom FluentResource, but I am not able to study another part of Spring framework due to time constraints

{ }
#endregion

public IConfigurableListableObjectFactory TemporaryInitObjectFactory { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I referred to this property in comment on SafeGetObjectFactory


namespace Spring.FluentContext
{
public static class ContextRegistrations
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to think more about this class. It drops the convention used in the library where you access those methods by IFluentApplicationContext and interfaces it implements. At the same time I understand that while the FluentApplicationContext was the only context configurable fluently, now you want to make all contexts to be configurable this way. Will come back to this.

/// </summary>
/// <returns>The internal ObjectFactory used by the ApplicationContext</returns>
/// <exception cref="System.InvalidOperationException">Cannot Access ApplicationContext in this state!</exception>
private static IConfigurableListableObjectFactory SafeGetObjectFactory(IConfigurableApplicationContext ctx)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method does not looks good :(

  • it violates Open-Closed Principle by casting interface to specific implementations,
  • it uses reflection to access private member of AbstractApplicationContext,
  • it uses the TemporaryInitObjectFactory property of the interface where it's name tells there is something wrong about it ;-) It almost looks like this should be some internal, maybe temporary state, but it's exposed to the public.

Can you think of the better implementation of this problem? Unfortunately, you did not provided any sample usages of those new fluent contexts, so I cannot check what problems you are exactly solving here and what would be the usability of those members. Please add examples, as it will help manage the project but also will help others to understand it.

Briefly, one alternative solution that comes to my mind is to make an interface exposing the ObjectFactory and implement it on all fluent contexts. You already have those implementations in place so it won't be hard.

The bigger question here I have is why you cannot just use the ctx.ObjectFactory as it is obviously there? Again, example would help.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole stuff tried to solve injecting Fluent config into AbstractXmlApplicationContext line 149 without the need of duplicating too many code from existing classes. I might found a better solution now, will give a try, please hold


namespace Spring.FluentContext
{
public class FluentXmlApplicationContext : XmlApplicationContext, IFluentConfigurableApplicationContext
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curious to see the use case for this class.

Why do you want to mix fluent with xml ?

  • do you want to have some config in xml and then other added fluently? I guess this can be done via composing contexts, i.e. new FluentApplicationContext(parentXmlContext),
  • do you want to register it in the app.config and then configure at runtime? What is the problem then of just constructing FluentApplicationContext at runtime and registering it ?

I still cannot grasp what does it solve...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can make easier switch from XML configs to Fluent as you don't need to register them all at once. Eg. I haven't found a way how to create a string object fluently. But probably this class was a semi-product during creating Web implementation.

{
public interface IFluentConfigurableApplicationContext
{
IConfigurableListableObjectFactory TemporaryInitObjectFactory { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left my thoughts about this TemporaryInitObjectFactory above. It sounds like a hack exposing some internals to the public. I am sure it can be done differently.


private IReferencingStage<TObject> MakeAlias(string objectId)
{
if (_ctx is IFluentConfigurableApplicationContext fcac)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, Open-Close Principle violation.

@mashbrno
Copy link
Author

Thanks for the hints, it allowed me to remove the bad temporarily iface solution. With the ContextRegistrations. I would remove the direct implementation in FluentApplicationContext, but as it would introduce breaking change I kept it as is and... duplicated the code. I might at least move the inner logic to extension class. What do you think?

@Suremaker
Copy link
Owner

Hello @mashbrno , I like those changes! The recent ones looks very good.

I would remove the direct implementation in FluentApplicationContext, but as it would introduce breaking change I kept it as is and... duplicated the code. I might at least move the inner logic to extension class. What do you think?

I believe it would be ok to move it out. I would even consider deleting the FluentApplicationContext altogether if it no longer bring any value. I would not worry about breaking changes, as this is a major version update anyway (so breaking change is ok according to semver rules), plus anyway it won't be that big difference for people using it.

Please still add some tests for the new classes and example usage with the FluentWebApplicationContext.

Btw, I wonder if there is any way of just using the standard web context and just use the extension methods to do the registrations? I guess you know better as you are actively working on it, but if there would be a way to avoid having FluentWebApplicationContext at all, then the extension methods will become applicable on any context (which would be great) - it's just a loose thought.

@kliszaq
Copy link

kliszaq commented Oct 13, 2020

Hi folks,
I really appreciate your input into the development of this feature.
The merge request seems to be almost finished in terms of the code clarity. What we are missing here is probably some small conflicts to be resolved to be done with it. Is there any chance @mashbrno you could finish the merge ?

kind regards,
Kamil

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

Successfully merging this pull request may close these issues.

3 participants