Pact-js: Enable Setting Of Popsicle maxBufferSize

[jpspringall]

Issue Classification

• [x ] Feature request
• [ ] Bug report

Feature Request

When generating pacts for use as a stub server the response can be bigger than the default set by Popiscle.
Popiscle supports the setting of maxBufferSize, however there currently no way of passing the value from pactJS.
I would like to add an option for this.
Just wondering anyone has an issue with this before I make the change?

Cheers

James

[mefellows]

Thanks James.

I don't recall seeing issues about this previously, but it doesn't seem unreasonable. Out of interest, how big are these payloads?

You also mention stub server, which has a specific meaning in the pact system (and shouldn't involve popscicle). I'm assuming you mean the in-process mock server used to write pact tests, and not the pact-stub-server binary you can use independently of Pact JS?

[jpspringall]

Hi @mefellows

Yes you are right, it has nothing to do with the pact-stub-server

Not sure be honest, the data used to write the pact tests, is auto generated through a powershell script.

The default in popiscle is as follows: var maxBufferSize = num(options.maxBufferSize, type === 'stream' ? Infinity : 2 * 1000 * 1000);

I've actually made the change (Still need to update comments and tests)

But a quick question, I've updated the constructor of the request class to take in a PopiscleOptions object.
Just wondered if you had a preference for this approach or whether you would prefer I just pass a number in?
I have seen it done both ways

export class Request {
  private readonly transport: TransportOptions

  constructor(config: PopiscleOptions) {
    this.transport = Popsicle.createTransport({
      maxBufferSize: config.maxBufferSize,
      rejectUnauthorized: false, // Need to tell node to ignore bad ssl cert
      type: "text",
    })
  }

[mefellows]

Awesome, thanks for having a good at a PR. Does it indeed fix your problem?

Popsicle is an implementation detail, so having it leak into the constructor is not ideal. That being said, it is only intended for use within the framework, so is easily refactorable.

I think the simplest option is to increase the buffer size for now, rather than do the heavy lifting to surface it to the top level API (which will be the Pact class via PactOptions).

@TimothyJones do you have thoughts on this?

I'd still like to understand the request payloads to fully appreciate the use case. I've not seen this problem in the 5+ years it's been in existence, so it's possible it's been used in a way that wasn't intended (but a good and valid case) or shouldn't be used.

[TimothyJones]

I agree, since popsicle is an implementation detail, I'm not really comfortable leaking its existence into the constructor (or any of the Pact interface, really).

I think the simplest option is to increase the buffer size for now, rather than do the heavy lifting to surface it to the top level API (which will be the Pact class via PactOptions).

I think I prefer this option. Would there be any downsides?

The ideal behaviour would be for this to be completely transparent to the user - I'm not completely across the context in this part of the code, but is it possible for us to detect the appropriate setting for this option and set it when needed?

Of course, having it work is definitely preferable to having the perfect interface. If we _have_ to expose the setting, I think I'd prefer setting a number in the options object.

[TimothyJones]

As an aside - while we're talking about the interface that is exposed, I noticed that we expose any in a few places where we could be more explicit. I went to fix this last night, and ended up down a bit of a rabbit hole of switching to eslint and fixing several other issues.

I mention this because in the process I noticed that we're due for a review of exactly what we expose in the primary export anyway. You can see the work I did on the types-and-lint branch - I'll open an issue for it, it's not ready to merge yet.

Anyway, this isn't strictly relevant, so I apologise for the digression.

Thanks @jpspringall for the proactive investigation and incoming fix! Issues with fixes are my favourite type of issue!

[mefellows]

I think I prefer this option. Would there be any downsides?

The only one I can think of is that if we set it too high and there is an accidental "I put too big a payload in my request" it might OOM somebody or crash a process. Which in my book is probably, because it's not a runtime framework but a dev time one. The number will likely also be fairly arbitrary.

The ideal behaviour would be for this to be completely transparent to the user - I'm not completely across the context in this part of the code, but is it possible for us to detect the appropriate setting for this option and set it when needed?

That's a good question. It's possible we might be able to detect it from the content-length of the interactions being built up in the test itself. In which case, it would be both accurate and transparent to the user. We'd probably need to account for other things that get buffered, so there will probably be some multiplier in there - at least it won't be as arbitrary.

As an aside - while we're talking about the interface that is exposed, I noticed that we expose any in a few places where we could be more explicit. I went to fix this last night, and ended up down a bit of a rabbit hole of switching to eslint and fixing several other issues.

Yick. Yep let's fix that indeed.

[jpspringall]

OK Do you want me to still raise a PR?

If you'd rather I didn't surface anything to the options, I could just set it to Infinity?

There's no great urgency for this on my side as we have a post install script that updates the request.js in node_modules.

Hackey, I know, but it's a dev tool and not going anywhere near production

[TimothyJones]

If you're able to make a PR that fixes the issue without exposing popsicle in the interface, that would be absolutely excellent.

(I've opened #528 to track the off-topic interface discussion I started)

[mefellows]

OK Do you want me to still raise a PR?

Yes please - we'd rather have your issue fixed than you have to patch it. Infinity 🤔 . It's a lot less arbitrary! Let's start with that, and we can whittle it down later (but again, if you could share your use case when the time is right, that would be helpful).

[jpspringall]

OK, Will do.

So basically it'll be a one line change with a unit test of data > than 2 * 1000 * 1000 to prove that the value has been set.

So my use case is as follows:

We use pact-stub-server to serve mocks in our local development and to run our acceptance tests for our SPA application.
And as @mefellows correctly says we use in-process mock server to write the pacts and generate the underlying json file for the pact-stub-server.

One of the apis that we call returns a mock that would normally returned by a CMS.
Obviously with the api being a call to a CMS, the data can be quite a bit bigger than for a 'normal' data api

90% of the responses come in below the 20KB default limit though the other 10% are greater than 20KB and so Ka Boom!

[mefellows]

Thanks, makes sense to me and doesn't sound too crazy.

Happy to make the simple change myself, but you've invested all of this time/effort, would love for you have have this contribution :)

[jpspringall]

More importantly, it gives me something to do in a completely pointless meeting :-)

[mefellows]

Much of Pact was created this way 😆

[jpspringall]

Hey,
So if i have understood correctly the unit tests don't all run correctly for the request.js
Because we testing against nock we don't actually test the popiscle settings.

I think i verified this by changing the SSL setting in the request.js and the test still passed.

The only think I can think of without it being too much is to make the values a public constant, and unit test against those to ensure that the settings are at least correct.

Thoughts?

[mefellows]

I'm OK for this change to not have any tests associated with it. The tests there are largely about scenario testing, not popsicle itself.

For a single line, I'd not overthink it (but, I _really_ appreciate that you are trying to put tests in there!)

I'd love to refactor the testing approach at some point... but not now.

[TimothyJones]

Fixed in #541 , released in v9.13.2.