Reduce code paths that could encode post data as the string undefined

[pauldambra]

Property based testing is a mechanism of sending many different combinations of arguments into some software and asserting on the general result of that.

Often when property based testing libraries find possible failures. They search for simpler examples of the failure to make them easy to understand. E.g. A library will try to identify not that failures are caused by "very long strings" but instead that they're caused by "strings longer than 6400 characters"

Imagine someone has implemented an algorithm which should always generate a number below 100

const fancyAlgorithm = (n) => n > 7 ? 99 : 101

It is easy to see in this simple example that the implementation doesn't meet the spec. But it isn't always easy to see that in real code.

using fast check we could test that the code has the property "regardless of the number given as input it generates a number less than 100"

import * as fc from 'fast-check'

fc.assert(
  fc.property(
    //generate number for input
    fc.integer(),
    // express the property
    (n) => fancyAlgorithm(n) < 100
  )
)

In this output below we can see FastCheck tried a negative number, identified a failure, and then tried zero to see if that still failed. When it did it reported zero as a counter example to the rule it had been given

In PostHog/posthog#4816 we see many instances of the API receiving an event or screen recording payload where the body is the literal value "undefined"

It is not easy to identify what is causing this

This PR uses property based testing to identify routes through the encodePostData method which could cause that output

Fast check allows the test below

describe('using property based testing to identify edge cases in encodePostData', () => {
    it('can use many combinations of typed arrays and options to detect if the method generates undefined', () => {
        assert(
            property(
                // the fast check framework runs the same test many times
                // these methods generate different values to use as input
                uint8Array(),
                boolean(),
                boolean(),
                // the test framework tries to be clever about generating values
                // or combinations that are more likely to cause a fail
                // e.g. very long strings or unusual string encodings for string inputs
                (data, blob, sendBeacon) => {
                // this method receives the generated parameters
                // uses them
                // and then returns true or false
                // false means that combination of parameters don't work as expected
                    const encodedData = encodePostData(data, { blob, sendBeacon, method: 'POST' })
                    // returns blob or string - ignore when it is not a string response
                    return (encodedData.indexOf && encodedData.indexOf('undefined') < 0) || true
                }
            ),
            { numRuns: 1000 } // default is 100 combinations
        )
    })
})

that property based test identifed a combination that generated the body data=undefined

    // this was identified as a combination that generated `data=undefined`, and the code could be fixed to stop that
    it('does not return undefined when blob and send beacon are false and the input is an empty uint8array', () => {
        const encodedData = encodePostData(new Uint8Array([]), { method: 'POST' })
        expect(typeof encodedData).toBe('string')
        expect(encodedData).toBe('data=')
    })

if both options.blob and options.sendBeacon were false ) and the input was a TypedArray then the undesirable input was generated.

Within the code was the check Array.isArray which fails for a TypedArray (thanks, JavaScript) and as a result the code would treat the input as an object and try to read a data property.

That can now be made impossible

NB that doesn't mean that the running application does travel that code path but that the code path was present and could have been travelled

[pauldambra]

Have shifted to "draft" status for clarity. Am convinced this is (the|a) source of the issue but not convinced I fully understand the consequence of the change yet

[mariusandra]

I think it looks good... just one annoying compatibility issue

[yakkomajuri]

We talked this over during a pairing session and made a few minor changes.

LGTM!