Add generators for URLs and domain names #18

wfscheper · 2020-11-01T00:45:38Z

This adds an RFC 1035 compliant domain name generator and an RFC 3986 compliant URL generator.

Closes #17

Signed-off-by: Walter Scheper [email protected]

flyingmutant · 2020-11-01T21:01:45Z

Thanks a lot for the PR! I'll try to review in the next couple of days. Can you please add your copyright on top of the new files?

wfscheper · 2020-11-03T00:39:21Z

Will do. I also realized I should add Examples, so I'll push them as well.

flyingmutant

Sorry for the delay! Finally managed to add some comments.

The thing I am concerned with the most is that only a tiny subset of possible URLs is generated. In general, I want rapid generators to produce every possible kind of valid data, to make sure that it is easy to catch overlooked corner cases in the things tested with rapid.

flyingmutant · 2020-11-08T08:02:41Z

tld.go

+ZW
+`
+
+var tlds = strings.Split(tldsByAlpha, "\n")


First and last elements of tlds are blank strings, is this intentional?

Good catch, no I need to runtldsByAlpha through TrimSpace.

flyingmutant · 2020-11-08T08:06:55Z

url.go

+	assertf(4 <= maxLength, "maximum length (%v) should not be less than 4, to generate a two character domain and a one character subdomain", maxLength)
+	assertf(maxLength <= 255, "maximum length (%v) should not be greater than 255 to comply with RFC 1035", maxLength)
+	assertf(1 <= maxElementLength, "maximum element length (%v) should not be less than 1 to comply with RFC 1035", maxElementLength)
+	assertf(maxElementLength <= 63, "maximum element length (%v) should not be greater than 63 to comply with RFC 1035", maxElementLength)


I'd prefer removing DomainOf entirely for now, and using named constants for 255 and 63.

flyingmutant · 2020-11-08T08:11:09Z

url.go

+}
+
+func (g *domainNameGen) String() string {
+	return fmt.Sprintf("Domain(maxLength=%v, mmaxElementLength%v)", g.maxLength, g.maxElementLength)


I try to make String() close to the code using to construct the generator, so after removing DomainOf it should just return Domain().

flyingmutant · 2020-11-08T08:16:23Z

url.go

+
+	var b strings.Builder
+	elements := newRepeat(1, 126, 1)
+	b.Grow(elements.avg())


b is unused below.

This was me cargo-culting from another implementation that used a repeat. I'll clean it up.

flyingmutant · 2020-11-08T08:20:31Z

url.go

+		domain = subDomain + "." + domain
+	}
+
+	return domain


RFC 1035 specifies that <domain> ::= <subdomain> | " ", should we support domain being space or not?

If it's in the spec, then we probably should, but I don't really grasp what " " as a domain even means. Guess I'll go back and re-read the spec.

flyingmutant · 2020-11-08T08:59:18Z

url_external_test.go

+)
+
+func TestURL(t *testing.T) {
+	pathEscapeRegex := regexp.MustCompile(`^[0-9A-Fa-f]{2}`)


Better to compile at the top-level; also t.Parallel() is missing for this test.

flyingmutant · 2020-11-08T09:08:17Z

url.go

+
+func (g *urlGenerator) value(t *T) value {
+	scheme := SampledFrom(g.schemes).Draw(t, "scheme").(string)
+	domain := Domain().Draw(t, "domain").(string)


Shouldn't we generate URLs with host component containing an IP (v4 or v6) address as well? That does not sound easy (especially given the IPv6 syntax variants), but otherwise we are omitting quite an important URL subset.

Good question. If I add those options, should I do so via public IPv4 and IPv6 Generators?

Yes, sounds good. Maybe a single generator of net.IP with an option like v6 bool?

Adding the IPv4 and IPv6 generators is pretty straightforward, but I'm not sure how to incorporate them into the URL generator.

I can think of three ways to do this:

One generator, URL(), that generates both DNS URLs and IP URLs

Two generators, URL() that generates DNS URLs and URLOf(schemes []string, domain *Generator).

Three generators, URL(), URLIPv4(), and URLIPv6()`

I think you're looking for the first option, but wanted to float the other two and see what you think.

I think going with just URL() for now is the safest option (because it can be extended in the future based on real feedback) -- in other cases we are guessing/speculating a bit about the use cases.

Great, thanks! I hope I'll be able to review/merge all this before the end of the week.

flyingmutant · 2020-11-08T09:08:39Z

url.go

+func (g *urlGenerator) value(t *T) value {
+	scheme := SampledFrom(g.schemes).Draw(t, "scheme").(string)
+	domain := Domain().Draw(t, "domain").(string)
+	port := IntRange(0, 2^16-1).


RFC does not limit the port range.

My intuition is that the RFC is refraining from specifying something that's out of the scope of the standard, rather than positively asserting that there is no limit on the range of valid ports.

Your call, but I think limiting the generation to the standard 16-bit valid port range is reasonable.

flyingmutant · 2020-11-08T09:11:35Z

url.go

+			StringOf(RuneFrom(nil, unicode.PrintRanges...)).Map(url.PathEscape),
+		).Draw(t, "path").([]string)...)
+
+	return url.URL{


We are not generating query or fragment parts, seems like quite a big omission.

I thought about that, but wasn't sure what to do.

With the very simple API currently exposed, there's no configurability of whether or not to generate query parameters or fragments. I suppose the consumer could use a Filter or Map to strip the parts they don't want. What are your thoughts?

Not sure, too -- but client-side stripping (if required) sounds like an OK option.

flyingmutant · 2020-11-08T09:12:47Z

url_example_test.go

+	// Z.CU
+	// r.ABBotT
+	// r.AcCoUNTaNT
+	// R.


R. is probably not what we wanted to generate?

Yeah, that's probably coming from the two empty string entries that are incorrectly included.

flyingmutant · 2020-11-11T14:30:12Z

By the way, any idea how to make CI work for this PR? It seems stuck.

wfscheper · 2020-11-11T21:07:15Z

I don't think anything is being scheduled for the PR, but I'm not sure why.

Edit: just looked at the workflow file, and there's no trigger for pull_requests. I can add that if you want. Here's one of my projects for comparision: https://github.com/wfscheper/stentor/blob/main/.github/workflows/build.yml

flyingmutant · 2020-11-12T16:28:30Z

Shouldn't "on (any) push" be enough?

Also, can you please try rebasing on top of current master? Maybe it can help CI to unstuck itself as well.

wfscheper · 2020-11-12T17:04:28Z

I think GitHub treats pushes to pull requests differently than pushes to "normal" branches. I'll try a rebase later today.

…

On Thu, Nov 12, 2020, 11:28 Gregory Petrosyan ***@***.***> wrote: Shouldn't "on (any) push" <https://github.com/flyingmutant/rapid/blob/master/.github/workflows/ci.yml#L3> be enough? Also, can you please try rebasing on top of current master? Maybe it can help CI to unstuck itself as well. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#18 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAPTM5GSAMBL5HO2WDFC7TSPQEL3ANCNFSM4TGGHHMA> .

flyingmutant · 2020-11-17T22:14:40Z

I've pushed the on pull_request change to the CI config, and created #20 to test it. Looks like CI change is working -- but the CI itself is failing on Go 1.13.

Can you please rebase this PR on top of current master and look into the CI failure? I think once the CI is happy I'll merge the PR, and fix the nitpicks (if I'll find something else to nitpick, that is) afterwards.

wfscheper · 2020-11-21T15:06:15Z

I'm at a loss on how to address the 1.13 issue. I think I may need to install go 1.13 and try stepping through the example with delve. I should have some time tonight.

wfscheper · 2021-09-02T03:08:23Z

Hey, apologies for leaving this hanging for so long. I see that you added some build tags to try and get around the example tests not lining up correctly. I also added a bash script to update the tld constant, which was out of date. Something to wire into a github action, perhaps?

flyingmutant · 2021-09-03T20:39:33Z

No problem, rapid is not changing too rapidly :-) I use build tags mainly to work around Unicode database updating from release to release.

Something to wire into a github action, perhaps?

I think just doing manual update every couple of releases might be good enough for now. I don't want to complicate the automation too much.

This adds a Generator for RFC 3986 compliant URLs, and two generators for RFC 1035 compliant domain names. Closes flyingmutant#17 Signed-off-by: Walter Scheper <[email protected]>

Signed-off-by: Walter Scheper <[email protected]>

Need to strip the leading and trailing newlines so we don't get empty strings in our top-level domain list.

This removes the configurable domain generation in favor of one that generates domains within the full rnage allowed by the specification.

To make the generated URL conform to how `url.Parse` works, Path and Fragment are set to the unescaped form. RawPath and RawFragment are left unset, and we simply rely on the URL struct to handle the escaping as needed.

These generators return `net.IP`, and are also used in generating URLs.

This adds a simple shell script to update the tld.go constant with the latest contents of https://data.iana.org/TLD/tlds-alpha-by-domain.txt

wfscheper · 2022-09-25T02:14:38Z

Hello! GitHub randomly reminded me about this abandoned PR of mine. Thought I'd take a bit and update it, and was pleasantly surprised by the new API using generics. Awesome stuff!

Only hiccup I had was the loss of the old Generator.Map() functionality. Is there an equivalent that I missed?

flyingmutant · 2022-09-27T14:48:45Z

Thought I'd take a bit and update it, and was pleasantly surprised by the new API using generics. Awesome stuff!

Yep, I was pleasantly surprised how little changes (conceptual, not LoC) were required to make rapid fully type-safe.

Only hiccup I had was the loss of the old Generator.Map() functionality. Is there an equivalent that I missed?

Methods can't have their own generic parameters, because of that Generator.Map() became top-level Transform.

flyingmutant · 2022-09-27T14:50:12Z

ip_addr.go

+	return fmt.Sprintf("IP(ipv6=%v)", g.ipv6)
+}
+
+func (g *ipGen) value(t *T) net.IP {


Maybe it is good idea to use netip? With generics we already require Go 1.18.

Makes sense. Done.

flyingmutant · 2022-09-27T14:50:49Z

url_example_test.go

+
+// String generation depends on the Unicode tables, which change with Go versions:
+//go:build go1.16
+// +build go1.16


No need, now we require Go 1.18 anyway.

This also removes the unnecessary build tag in the example tests.

wfscheper force-pushed the feat-url-generator branch from 80bed69 to 86653bc Compare November 3, 2020 00:48

flyingmutant reviewed Nov 8, 2020

View reviewed changes

wfscheper force-pushed the feat-url-generator branch from b8127fb to 0a22d63 Compare November 11, 2020 00:59

wfscheper force-pushed the feat-url-generator branch from 0a22d63 to 5122cc0 Compare November 13, 2020 00:28

flyingmutant mentioned this pull request Nov 17, 2020

URL-related generators, once more #20

Closed

wfscheper force-pushed the feat-url-generator branch from 5122cc0 to 40936f2 Compare November 19, 2020 01:42

wfscheper force-pushed the feat-url-generator branch from 3f9a86a to 07032e1 Compare September 2, 2021 03:00

wfscheper and others added 10 commits September 24, 2022 21:29

Add generators for URLs and domain names

be1e99a

This adds a Generator for RFC 3986 compliant URLs, and two generators for RFC 1035 compliant domain names. Closes flyingmutant#17 Signed-off-by: Walter Scheper <[email protected]>

Add examples for URL and domain generators

e4d47fd

Signed-off-by: Walter Scheper <[email protected]>

Update copyright headers

0130cb5

Signed-off-by: Walter Scheper <[email protected]>

Trim tlds list before splitting

131a8ce

Need to strip the leading and trailing newlines so we don't get empty strings in our top-level domain list.

Simplify domain generation

c8b1ad2

This removes the configurable domain generation in favor of one that generates domains within the full rnage allowed by the specification.

URL generator also generates query and fragment

e7bad3e

To make the generated URL conform to how `url.Parse` works, Path and Fragment are set to the unescaped form. RawPath and RawFragment are left unset, and we simply rely on the URL struct to handle the escaping as needed.

Add IPv4 and IPv6 generators

0a6a872

These generators return `net.IP`, and are also used in generating URLs.

Fix URL example tests

18ad723

Add script to update top-level domains

4846a6a

This adds a simple shell script to update the tld.go constant with the latest contents of https://data.iana.org/TLD/tlds-alpha-by-domain.txt

Update to use new generic API

1499ace

wfscheper force-pushed the feat-url-generator branch from 07032e1 to 1499ace Compare September 25, 2022 02:11

flyingmutant reviewed Sep 27, 2022

View reviewed changes

wfscheper added 2 commits September 27, 2022 20:18

IPv4 and IPv6 return netip.Addr

b91d28f

Use Transform for URL generation

8aaba74

This also removes the unnecessary build tag in the example tests.

Add generators for URLs and domain names #18

Are you sure you want to change the base?

Add generators for URLs and domain names #18

Conversation

wfscheper commented Nov 1, 2020 • edited Loading

flyingmutant commented Nov 1, 2020

wfscheper commented Nov 3, 2020 • edited Loading

flyingmutant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyingmutant commented Nov 11, 2020

wfscheper commented Nov 11, 2020 • edited Loading

flyingmutant commented Nov 12, 2020

wfscheper commented Nov 12, 2020 via email

flyingmutant commented Nov 17, 2020

wfscheper commented Nov 21, 2020

wfscheper commented Sep 2, 2021

flyingmutant commented Sep 3, 2021

wfscheper commented Sep 25, 2022

flyingmutant commented Sep 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wfscheper commented Nov 1, 2020 •

edited

Loading

wfscheper commented Nov 3, 2020 •

edited

Loading

wfscheper commented Nov 11, 2020 •

edited

Loading