test: fieldKeyParser: make clearer assertions about url manipulation

[alxndrsn]

No description provided.

[alxndrsn]

It looks like removal of the /v1 prefix is applied inconsistently.

[matthew-white]

Instead of having createRequest() automatically specify originalUrl by prepending /v1 to url, could we just have individual tests specify originalUrl when they need to? I only see three sets of tests that specify url at all:

• versionParser()
• fieldKeyParser()
• odataPreprocessor()

I don't think versionParser() needs access to originalUrl. For the tests of odataPreprocessor(), I don't think it makes a ton of sense to prepend /v1 to url: the urls are things like '/odata.svc?$format=json'.

What do you think about adding a utility function just for the tests of fieldKeyParser()? Those seem to be the only ones that actually use originalUrl. I'm thinking of something similar to createPreVersionParserRequest():

const createFieldKeyParserRequest = ({ url, ...props }) =>
  if (originalUrl) throw new Error('Should not specify originalUrl');
  return createRequest({ url, originalUrl: `/v1${url}`, ...props });
};

Maybe we could come back to the idea of automatically specifying originalUrl if/when we have more tests that need that.

[alxndrsn]

could we just have individual tests specify originalUrl when they need to?

We could, but it's allows people to forget to do specify it.

For the tests of odataPreprocessor(), I don't think it makes a ton of sense to prepend /v1 to url: the urls are things like '/odata.svc?$format=json'.

It's true, but to understand that these tests do not need originalUrl, you need to check what each preprocessor is doing internally. And you'd need to reconfirm that when modifying preprocessor code. The approach in this PR allows writing and testing of preprocessors in an environment that more closely matches reality by default, rather than requiring explicit opt-in.

[matthew-white]

I see what you're saying, but I think there's potential for confusion if our version of createRequest() started to diverge more so from node-mocks-http. When I was first reviewing this PR, I didn't notice at first that createRequest() had been modified, and I spent a few minutes scratching my head as to how it was possible that /v1 was being added to originalUrl. I thought I was misunderstanding something about the preprocessors.

We don't have that many preprocessors, and there's only one preprocessor where there's a problem with the tests. I think it'd be better to wait to modify createRequest() in this way until it's clearer that that's what we'd want for future preprocessors. If we were to add another preprocessor before fieldKeyParser(), then we wouldn't want createRequest() to prepend /v1 to originalUrl.

Just to throw out another idea, what do you think about wrapping the request in a proxy that gates the originalUrl property? If a preprocessor tries to access originalUrl, but no originalUrl was specified to createRequest(), then the proxy could throw an error. I think that would avoid possible confusion while still keeping an eye on originalUrl. I'm thinking of something like:

const guardOriginalUrl = {
  get: (request, prop) => {
    if (prop === 'originalUrl')
      throw new Error('no originalUrl was passed to createRequest()');
    return request[prop];
  },
  set: (request, prop, value) => {
    request[prop] = value;
    return true;
  }
};

const createRequest = options => {
  // ...
  const request = wrapped.createRequest({ ...options, query: qs.parse(search.substr(1)) });
  return options.originalUrl != null ? request : new Proxy(request, guardOriginalUrl);
};

[alxndrsn]

I see what you're saying, but I think there's potential for confusion if our version of createRequest() started to diverge more so from node-mocks-http.

I think node-mocks-http behaves surprisingly with respect url and originalUrl - if originalUrl is not specified explicitly, it defaults to url:

https://github.com/eugef/node-mocks-http/blob/master/lib/mockRequest.js#L65

In our case, in part because various middlewares are rewriting both values at different times, this leads to misleading tests for anyone not familiar with the defaults for node-mocks-http createRequest() function.

Maybe the clearest approach would be to require both originalUrl and url to be supplied explicitly to createRequest()?

[alxndrsn]

Maybe the clearest approach would be to require both originalUrl and url to be supplied explicitly to createRequest()?

@matthew-white I've implemented this alternative approach at #1302