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

Calling AddRule can modify DefaultList #141

Closed
guliyevemil1 opened this issue Aug 13, 2018 · 4 comments
Closed

Calling AddRule can modify DefaultList #141

guliyevemil1 opened this issue Aug 13, 2018 · 4 comments
Assignees
Labels

Comments

@guliyevemil1
Copy link
Contributor

guliyevemil1 commented Aug 13, 2018

It would have been nice if DefaultList's type was List instead of *List, but I realize that changing it now would break backwards compatibility. I wanted to add a few of my own custom domains to the list but I realized that I would be modifying DefaultList's rules by doing so. My work around is to create my own global variable like so:


var (
	defaultList = new(publicsuffix.List)

	customSuffices = map[string]bool{
		// my list here
	}
)

func init() {
	*defaultList = *publicsuffix.DefaultList
	for customSuffix := range customSuffices {
		defaultList.AddRule(publicsuffix.MustNewRule(customSuffix))
	}
}

But I think it's easy to overlook for other users of the library. Maybe the best solution is to create a NewDefaultList() function?

@weppos
Copy link
Owner

weppos commented Aug 14, 2018

Thanks, I'll look into that.

but I realize that changing it now would break backwards compatibility.

I don't consider the library 1.0 yet because I'm still fine tuning. I also want to enhance the lookup algorithm (thanks for your patch) before considering it 1.0.

For this reason, I'm willing to take the risk of committing breaking changes. I expect most apps to vendor the dependency, and/or the compiler to complain.

@guliyevemil1
Copy link
Contributor Author

Okay, very cool :) I have a few ideas on how it can be improved. I'll submit a PR to see what you think.

@weppos weppos changed the title calling AddRule can modify DefaultList Calling AddRule can modify DefaultList Aug 23, 2018
@weppos
Copy link
Owner

weppos commented Aug 23, 2018

Sorry, it took me a while to switch context. I was looking into this issue and I actually realized that what you are expecting (unless I'm mistaken) is the expected behavior.

DefaultList is the default list that this lib operates on when you don't specify a custom list. Or when you want to assign a custom list as the default one.

It is not default in the sense of "frozen", but default in the sense of the fallback one.

It somehow works as the DefaultClient in net/http:

https://github.com/golang/go/blob/c5d38b896df504e3354d7a27f7ad86fa9661ce6b/src/net/http/client.go#L107-L108

I believe though I see what you are staying. Since the "shipped compiled list" is also by coincidence the default list, there is a possibility that modifying the "default" list would permanently alter the state of the "shipped list" at least until the process is rebooted.

So a potential solution would be to have a compiled list that is then assigned as default list (cloned) that you can alter, but at any time you can discard the changes and restart from the compiled list. Is it what you would expect?

@guliyevemil1
Copy link
Contributor Author

Yes, precisely. Currently, there is no way to clone the default list and add my own custom URLs. I am forced to modify the DefaultList which may lead to unexpected behavior if another part of the code in the same binary is relying on it as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants