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

be able to control randomness of boundary headers #276

Merged
merged 21 commits into from
Feb 10, 2023

Conversation

xoba
Copy link
Contributor

@xoba xoba commented Feb 5, 2023

in order to create reproducible email messages, i needed reproducible uuid strings which are used for boundary headers. therefore, i added a new top-level method func SetRandom(r *rand.Rand) which causes the UUID function to use that given Rand object. for instance, in my project i call it like enmime.SetRandom(rand.New(rand.NewSource(1))). in the absence of such a call, it is initialized like time.Now().UnixNano() as before.

@jhillyerd
Copy link
Owner

jhillyerd commented Feb 5, 2023

I'm in favor of this feature, but I think we need to move away from using a global variable for the random source. If your code, or its tests, run in parallel, you will clobber the random seed and will not get deterministic output.

Perhaps we can store a randsource reference & mutex in the builder, then pass it into UUID()?

@xoba
Copy link
Contributor Author

xoba commented Feb 5, 2023

sure, thanks, that was my first thought as well, but then saw that threading it through the code might be cumbersome, having to go through enmime.Part type as well? but if you're still in favor of doing it that way, i could update this pull request accordingly.

@jhillyerd
Copy link
Owner

I just threaded Parser into Part a couple days ago, it may be possible to leverage it and the Options stuff to get the random source into the right place.

@xoba
Copy link
Contributor Author

xoba commented Feb 6, 2023

i've updated this pull request with a specific MailBuilder option instead, please take a look and let me know what you think! thanks again.

@jhillyerd
Copy link
Owner

Overall I think this is a good approach, but I don't think we should add the functional options pattern to a builder, as those are two ways of doing the same thing; mixing paradigms will be confusing.

It also breaks the re-usability pattern I want the builder to support, where you can setup a root builder a specific way, and then re-use it for multiple emails. There is no way to reset the rand source for each of those.

Let's just define a func (p MailBuilder) RandSource(r *rand.Rand) MailBuilder

@xoba
Copy link
Contributor Author

xoba commented Feb 6, 2023

got it thanks, i'll re-work it that way.

@xoba
Copy link
Contributor Author

xoba commented Feb 7, 2023

please take a look at updated request which i believe satisfies your requirements, thanks!

Copy link
Owner

@jhillyerd jhillyerd left a comment

Choose a reason for hiding this comment

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

Sorry, I know a lot of back and forth on this one, but it's very close

internal/stringutil/uuid.go Outdated Show resolved Hide resolved
randoption_test.go Outdated Show resolved Hide resolved
randoption_test.go Outdated Show resolved Hide resolved
randoption_test.go Outdated Show resolved Hide resolved
@xoba xoba requested a review from jhillyerd February 9, 2023 19:03
@jhillyerd jhillyerd merged commit 2161503 into jhillyerd:master Feb 10, 2023
@jhillyerd
Copy link
Owner

Thank you!

@xoba
Copy link
Contributor Author

xoba commented Feb 10, 2023

thanks!

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.

None yet

2 participants