Rule provider not initialized breaks dependency injection principles

[merijndejonge]

In order to make use of DomainParser the rule provider it consumes must be initialised by calling the Build() method or else an exception is thrown (DomainDataStructure is not available).

In order to call the Build() method in an aspnet context, some specific, non standard code needs to be executed in the startup class, which, as a side effect, requires that dependencies have to be registered as singleton. When rule providers are more advanced than the ones provided this github repo, for instance when data bases are involved, this may lead to a far from optimal setup.

Why is the Build() method not called by the DomainParser class. That is, rather than throwing an exception when _ruleProvider.GetDomainDataStructure() returns null, why not calling _ruleProvider.Build()?

I offer my help to discuss the design to see if it can be improved for more advance usage scenarios.

[merijndejonge]

Any thoughts on this?

[tinohager]

I don't have time to examine the topic in detail right now, I'll take a look at it when I get the chance

[FlorianHockmann]

We just ran into the same issue when trying to update to the latest version. +1 on this suggestion from @merijndejonge:

Why is the Build() method not called by the DomainParser class. That is, rather than throwing an exception when _ruleProvider.GetDomainDataStructure() returns null, why not calling _ruleProvider.Build()?

I think that it's also confusing that we need to manually initialize the RuleProvider although we only want to use the DomainParser and wouldn't interact at all directly with the RuleProvider.
This now means that a class using the DomainParser needs to add another dependency on the RuleProvider and then initialize it to make the DomainParser work and this only works because the DomainParser will use the same RuleProvider instance due to the singleton config of the dependency injection.

Just letting the DomainParser initialize the RuleProvider itself like @merijndejonge suggested would solve this.

[merijndejonge]

This is how I now solved it. A bit clumsy, but it makes the code more DI-compliant until the library itself is improved:

public class DiDomainParser(IRuleProvider ruleProvider, IDomainNormalizer? domainNormalizer = null)
    : IDomainParser
{
    private readonly DomainParser _domainParser = new(ruleProvider, domainNormalizer);

    public DomainInfo? Parse(string fullyQualifiedDomainName)
    {
        if (ruleProvider.GetDomainDataStructure() == null)
        {
            ruleProvider.BuildAsync().Wait();
        }
        return _domainParser.Parse(fullyQualifiedDomainName);
    }

    public DomainInfo? Parse(Uri fullyQualifiedDomainName)
    {
        if (ruleProvider.GetDomainDataStructure() == null)
        {
            ruleProvider.BuildAsync().Wait();
        }
        return _domainParser.Parse(fullyQualifiedDomainName);
    }

    public bool IsValidDomain(string fullyQualifiedDomainName)
    {
        if (ruleProvider.GetDomainDataStructure() == null)
        {
            ruleProvider.BuildAsync().Wait();
        }

        return _domainParser.IsValidDomain(fullyQualifiedDomainName);
    }
}

[tinohager]

It is not possible for you to initialize your logic like I do here?

var ruleProvider = app.Services.GetService<IRuleProvider>();
if (ruleProvider != null)
{
    await ruleProvider.BuildAsync();
}

https://github.com/nager/Nager.PublicSuffixService/blob/main/src/Nager.PublicSuffixService/Program.cs

[merijndejonge]

It requires that a rule provider including dependencies are all registered as singleton services. Why would you want to pose such a strong requirement on a rule provider?

[bkarakaya01]

@tinohager First of all thank you for what you've done here. As I see, this is a common problem.

In my opinion, user should not have to dig in the details about any provider object to create a simple DomainParser class as @FlorianHockmann states. Or at least a meaningful error message would also be great, such "DomainDataStructure is not available, IRuleProvider.BuildAsync has to be called in order to call DomainParser.Parse() method."

[tinohager]

@merijndejonge @FlorianHockmann @bkarakaya01

If we call the creation logic in the BaseRuleProvider during instantiation, we can react less easily to any problems that may arise.

For example, we lose the option of canceling the process via the CancellationToken. We also cannot ensure that the process was successful.

What would you expect in these cases?

namespace Nager.PublicSuffix.RuleProviders
{
    /// <summary>
    /// BaseRuleProvider
    /// </summary>
    public abstract class BaseRuleProvider : IRuleProvider
    {
        /// <summary>
        /// Parsed public suffix list rules
        /// </summary>
        protected DomainDataStructure? _domainDataStructure;

        public BaseRuleProvider()
        {
            this.BuildAsync(ignoreCache: false).ConfigureAwait(false).GetAwaiter().GetResult();
        }

[bkarakaya01]

I'm aware of the situation here, but there should be a way to force the developer to call the related methods and ensure that build was success. Maybe we should discuss on this.

[merijndejonge]

I would, in general, not do complex things in a constructor. I think that is against good coding practice.
If there really is a need for an explicit Build method, it should be part of a contract that belongs to the rule provider and only of concern to parties which directly interact with the rule provider. The obvious (and perhaps only) party that is using a rule provider is the domain parser. So, this guy should be able to understand the contract of the rule provider. That is why I proposed to let the domain parser make the call to the build method. I still recommend this approach.

As a result, users of the domain parser don't need to interact directly with the rule provider. Dealing with cancelation tokens and/or error handling can also be handled by the domain parser.