Change Default Content-Type for POST Requests to 'application/json'

[tedrand]

Type of change

• Bugfix
• Feature
• New bidder adapter
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Other

Description of change

This change allows POST bid requests to be treated as JSON, by setting the Content-Type of the header.

While the docs for how to make a Prebid 1.0.0 adapter say that POST requests are treated as JSON, they are actually being treated as plain text currently.

Other information

I asked @mkendall07 about this bug and was told that it was worth putting in a pull request for.

[mkendall07]

Thanks for the PR. I think we want to make this configurable however. Reason being that if it's application/json then CORS preflight request is required (additional round trip). text/plain doesn't have this requirement.

[tedrand]

Thanks, is the intention for 1.0.0 then for the endpoint to accept the POST request JSON payload in plain text format?

[tedrand]

how do you feel about an optional 'contentType' param on the spec object?

So it would be,

contentType: spec.contentType || 'text/plain'

[dbemiller]

The spec probably isn't a great place, because some adapters make several server requests. If a bidder wanted to make two requests with two different content types, they'd be stuck.

It would fit in pretty nicely on the ServerRequest object, though.

[tedrand]

works for me, thanks for helping out. We are just trying to avoid our current method of sending JSONP.

[tedrand]

I'll get a new PR in shortly.

[dbemiller]

yep yep... the internet thanks you :)

[tedrand]

Hmm.... can't tell why it is still failing the tests.

[dbemiller]

I know it's a small change... but I still think there should be a unit test for this, to prevent regressions.

[dbemiller]

Ony minor thing... how do you feel about request.options.contentType, where options is an object?

Reason being: this could become Object.assign({}, theseDefaultPostOptions, requests.options), and then bidders will be able to edit any of the ajax options that they wanted.

Another benefit is that the JSDoc types would be reusable. The fourth argument to ajax would have the same typedef as the ServerRequest.options object.

[tedrand]

Makes sense to me. Should the Object.assign() route only be implemented/tested for the POST ajax call? Or would there be a reason to pass options on the GET too?

[dbemiller]

hmm... I guess I'd have a small preference towards doing it for both, just because parallelism makes the behavior easier for people to remember.

It's a pretty weak preference, though. At worst, the first adapter which passes options to GET will find out that it doesn't work, and then they'll just have to add it later.

[dbemiller]

What if someone wanted to override withCredentials: false?

Is there any particular reason you're trying to delete null & undefined here?

[tedrand]

Ah yeah, that was dumb my bad.

[dbemiller]

I don't think so... I just ran it myself, and this test fails:

    it('should allow falsey options in the overrides', () => {
      const bidder = newBidder(spec);
      const url = 'test.url.com';
      const data = { arg: 2 };
      const options = { withCredentials: false }
      spec.isBidRequestValid.returns(true);
      spec.buildRequests.returns({
        method: 'POST',
        url: url,
        data: data,
        options: options
      });

      bidder.callBids(MOCK_BIDS_REQUEST);

      expect(ajaxStub.calledOnce).to.equal(true);
      expect(ajaxStub.firstCall.args[0]).to.equal(url);
      expect(ajaxStub.firstCall.args[2]).to.equal(JSON.stringify(data));
      expect(ajaxStub.firstCall.args[3]).to.deep.equal({
        method: 'POST',
        contentType: 'text/plain',
        withCredentials: false        // The test succeeds if this is true
      });
    });

Pretty sure you're deleting all false-y properties from the options object, so they don't override properly.

[dbemiller]

LGTM

[dbemiller]

Tiny optimization: you don't need to Object.assign({}, defaults, overrides) here. Since the defaults are defined right here, Object.assign(defaults, overrides) should work just fine, and allocate one fewer object.

Definitely shouldn't hold up the merge though.