Modify fc.webPath to be able to generate paths starting with //

[astlouisf]

🚀 Feature Request

fc.webPath should be able to generate paths starting with '//'.

The following grammar derivation (starting from the path definition in rfc3986) suggests that they should be valid paths

path = path-abempty / path-absolute / path-noscheme / path-rootless / path-empty
     = path-abempty
     = *( "/" segment )
     = ( "/" segment ) *( "/" segment )
     = ( "/" segment ) ( "/" segment ) *( "/" segment )
     = "/" segment "/" segment *( "/" segment )
     = "/" *pchar "/" segment *( "/" segment )
     = "/" "/" segment *( "/" segment )

I haven't dug in the code yet, but the current implementation doesn't seem to ever produce paths matching this pattern:

it('never generate //', () => {
  fc.assert(fc.property(fc.webPath(), (pathname) => {
    expect(pathname).not.toMatch(/^\/\/.*/);
  }), {numRuns: 100000});
});

Motivation

It would catch more bugs :) and seems like a current blind spot of the webPath implementation.

Example

The following test should fail (sometimes).

it('preserve origin', () => {
  fc.assert(fc.property(fc.webPath(), fc.webUrl(), (pathname, urlString) => {
    const url = new URL(urlString);
    expect(new URL(pathname, urlString).origin).toBe(url.origin);
  }));
});

[gruhn]

I looked into this and I was surprised initially. Looks like webPath is just constructed from a joined fc.array(fc.webSegment()) and since webSegment can generate empty strings, webPath should totally be able to generate "//". However, looks like the default size parameter for the array is xsmall so only very few path segments are generated. I computed some statistics and looks like webPath pretty much never generates more than 2 segments (which would be necessary to generate "//"):

fc.statistics(fc.webPath(), path => String(path.split('/').length >= 3), 10_000)
// ==> false..100.00%

However, if I manually increase the size parameter (fc.webPath({ size: '+1' })) I can generate "//" sometimes and make both your tests fail.

[astlouisf]

I've experimented a bit more and it doesn't seem to generate paths directly starting with a segment either

fc.statistics(fc.webPath({size:'+1'}), (path) => { return path.match(/^[^/]/) ? 'initial segment': 'absolute or empty'; });
// ==> absolute or empty..100.00%

this is expected considering

fast-check/packages/fast-check/src/arbitrary/_internals/mappers/SegmentsToPath.ts

Line 6 in 9c51766

safeMap(segments, (v) => `/${v}`),

but it raises the question about which part of RFC 3986 fc.webPath corresponds to since path-rootless would allow an initial segment.

[gruhn]

@dubzzz could we have

  return safeJoin(
    segments,
    '/',
  );

instead of

  return safeJoin(
    safeMap(segments, (v) => `/${v}`),
    '',
  );

That would still allow web paths with leading "/" when the first segment is an empty string, right?

[dubzzz]

I believe segments don't come with the /, so we might only have one leading slash with that approach and no others.

That said, that might be a good idea.

[gruhn]

I mean if we the array of segments looks like this: ["", "", "foo", "bar"] and we join it with / we get: //foo/bar, no?

[dubzzz]

Oh yes. In theory (except bug), it should

[dubzzz]

I quickly looked into the code and definitely I feel that // should be produced at some point. I'll try to get what goes wrong. From my quick scan of the code I'd expect it to work fine but apparently it doesn't.

[gruhn]

Also just checked when I change segmentsToPathMapper to

safeJoin(segments, '/');

the distribution changes quite a bit:

function countLeadingSlashes(path: string): number {
  if (path.charAt(0) === '/') {
    return 1 + countLeadingSlashes(path.slice(1))
  } else {
    return 0
  }
}

fc.statistics(fc.webPath({ size: '+1' }), path => countLeadingSlashes(path) + " leading slashes", 100_000)   
// 0 leading slashes..92.74%
// 1 leading slashes...6.72%
// 2 leading slashes...0.49%
// 3 leading slashes...0.04%
// 4 leading slashes...0.01%

So while previously there was always at least one leading slash, now there is no leading slash most of the time. Not sure if that's desired.

Side note: how do you treat distribution changes like this? Is that a minor/major/patch/etc version bump?

Also not sure if this is completely faithful to the spec now.

[astlouisf]

If I understand correctly, fc.webPath used to be an internal arbitrary that was used to implement fc.webUrl. This lead me to believe that paths generated by fc.webPath correspond only to path-abempty from the spec since fc.webUrl seems to always generate an authority.

This would mean that a leading slash is expected.

[dubzzz]

Well actually from my understanding it's theoretically feasible to get such "//" value out of the current version of the implementation. I'm currently trying to understand why it seems to never happen.

The code:

fc.array(fc.webSegment({ size: "small" }), { size: "small" }) //1.
  .map(data => data.map((v) => `/${v}`).join('')) //2.

Should be able to create an instance being equal to ["",""] in 1. And then to result into //.

Currently checking what I'm missing.

[dubzzz]

So strange, I'll have to dig further:

// Fails (expected)
fc.assert(
  fc.property(
    fc.array(fc.webSegment({ size: "small" }), { size: "small" }),
    p => {
      return !(p[0] === p[1] && p[0] === '');
    }
  ), {numRuns:1000000}
);

// Fails (expected)
fc.assert(
  fc.property(
    fc.array(fc.webSegment({ size: "small" }), { size: "small" })
      .map(data => data.map((v) => `/${v}`).join('')),
    p => {
      return !p.startsWith('//');
    }
  ), {numRuns:1000000}
);

// No fail (not expected)
fc.assert(
  fc.property(
    fc.webPath(),
    p => {
      return !p.startsWith('//');
    }
  ), {numRuns:1000000}
);

[dubzzz]

I made a typo: it should be array(webSegment({ size: 'small' }), { size: 'xsmall' })

[dubzzz]

Ok, so by adding the "+1" for the size requested to the webPath I get the two //.

fc.assert(
  fc.property(
    fc.webPath({size:'+1'}),
    p => {
      return !p.startsWith('//');
    }
  ), {numRuns:1000000}
);

It also makes your initial test fails:

fc.assert(
  fc.property(
    fc.webPath({size:'+1'}),
    p => {
      return !/^\/\/.*/.test(p);
    }
  ), {numRuns:1000000}
);

---

Regarding your question: yes it used to be

If I understand correctly, fc.webPath used to be an internal arbitrary that was used to implement fc.webUrl.

---

Regarding rootless, maybe we should add a withRootLess option to support this need.

but it raises the question about which part of RFC 3986 fc.webPath corresponds to since path-rootless would allow an initial segment.

[dubzzz]

As justified by @astlouisf, I think we need the first / because of webUrl today. So the fix consisting into doing the following:

safeJoin(segments, '/');

Is probably not enough alone. as such it would be a breaking for the contract that users may expect from webPath (given today they can see it always starts by a /). I'd go for an option to include root-less for this major and possibly drop it and make it the default for the next major of the library.

[astlouisf]

@dubzzz Thank you for investigating this!

I'm not sure root-less should necessarily be added as part of fc.webPath (even though I brought it up 😅). The more I dug into RFC 3986 the more I realized that paths really depend on contexts and, sometimes, those contexts are conflicting.

If other type of paths are desired, I think specific arbitraries should be implemented.

In any cases, I also think the documentation for fc.webPath could potentially be improved by specifying that fc.webPath generates path compatible with path-abempty.

[dubzzz]

We can definitely just update the documentation then. Opened to PR clarifying it

[astlouisf]

I still think fc.webPath should generate path of the form //* in it's default configuration.

[dubzzz]

Good point, I have a non breaking change in mind to handle that need in a timely manner. I'll deal with it.

For the update of the documentation, I prefer letting you doing it. You spent time on this issue so I'll be happy to have your PR and add you as a contributor 👍

[dubzzz]

Released at 3.18.0. Feel free to re-open in case it does not fix the original issue.