Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(rules): fix header concatenation #394

Merged
merged 2 commits into from
Mar 18, 2022

Conversation

jan-law
Copy link
Contributor

@jan-law jan-law commented Mar 18, 2022

Fixes #390

When two headers objects are passed to fromFetch, the last header object listed (config.headers) seems to replace the previous headers object (login.getHeaders). This appends the correct Content-type, but removes the Authorization header from the request. Appending the config.headers into the original headers object, then passing in the updated headers object last will preserve all the headers.

@jan-law jan-law added the fix label Mar 18, 2022
@jan-law jan-law requested a review from andrewazores March 18, 2022 14:51
@jan-law jan-law force-pushed the auto-rules-basic-auth branch from be7e71c to fb15642 Compare March 18, 2022 14:57
Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Is there any way to handle this merging more generically, so it isn't a special case for the headers in particular?

https://lodash.com/docs/4.17.15#merge may be useful here?

@jan-law
Copy link
Contributor Author

jan-law commented Mar 18, 2022

I know _.merge() is supposed to deep copy objects, but I'm not sure what's happening to the Headers here?

Whichever way I arrange the dest and source from _.merge(dest, [source]), the Content-type header from config.headers disappears. The final request contains the other merged fields perfectly fine.

All of these return Request failed (415 Unsupported Media Type): "reason":"Bad content type: text/plain;charset=UTF-8":

let request = {
          credentials: 'include',
          mode: 'cors',
          headers,
        } as RequestInit;

        _.merge(config, request);

        return fromFetch(`${this.login.authority}/api/${apiVersion}/${path}`, config);
let request = {
          credentials: 'include',
          mode: 'cors',
          headers,
        } as RequestInit;

        _.merge(request, config);

        return fromFetch(`${this.login.authority}/api/${apiVersion}/${path}`, request);
let request = {
          credentials: 'include',
          mode: 'cors',
          headers: headers,
        } as RequestInit;

        _.merge(request, config);

        return fromFetch(`${this.login.authority}/api/${apiVersion}/${path}`, request);
let request = {
          credentials: 'include',
          mode: 'cors',
          headers: headers,
        } as RequestInit;

        if(!!config && !!config.headers) {
          _.merge(request.headers, config.headers);
        }

        _.merge(config, request);

        return fromFetch(`${this.login.authority}/api/${apiVersion}/${path}`, config);

@andrewazores
Copy link
Member

Ah okay, maybe it isn't working as expected because the headers object isn't just a plain Object but the Headers type.

@jan-law
Copy link
Contributor Author

jan-law commented Mar 18, 2022

I think I need to call headers.set(key, value) when merging the two objects.

Style-wise, I could use mergeWith and a customizer like below. What do you think?

function customizer(dest, src) {
          if (dest instanceof Headers && src instanceof Headers) {
            Object.entries(src).forEach(([k, v]) => {
              dest.set(k, v);
            });
          }
          return dest;
        }

  _.mergeWith({
    credentials: 'include',
    mode: 'cors',
    headers: headers,
  } as RequestInit, config, customizer);

  return fromFetch(`${this.login.authority}/api/${apiVersion}/${path}`, config);

@andrewazores
Copy link
Member

The docs say that mergeWith returns the merged result and also mutates the first argument - if that's accurate then I think your snippet is missing something because it looks like the config passed to fromFetch is just going to be the original, unmodified config object, isn't it?

config = _.mergeWith() would be enough to do it, I think. Or else extract the RequestInit to a local variable and pass that reference in to _.mergeWith and then pass that same reference to the now-mutated object to fromFetch?

@andrewazores andrewazores merged commit 84d64a4 into cryostatio:main Mar 18, 2022
@jan-law jan-law deleted the auto-rules-basic-auth branch March 22, 2022 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating automated rule with BasicAuthManager returns HTTP 401
2 participants