Consider making HttpClient features opt-in #44

Closed
wictorwilen opened this Issue Aug 21, 2016 · 13 comments

Projects

None yet

7 participants

@wictorwilen
Contributor

The HttpClient (@microsoft/sp-client-base) always add a "odata-version":"4.0" header to all requests. This makes requests to for instance the SharePoint Search API endpoint to fail (HTTP 500).

Please remove this automatic addition of the header.

@vman
Contributor
vman commented Aug 22, 2016

For now, you can manually add the odata version 3.0 in the headers:

const reqHeaders = new Headers();
    reqHeaders.append('odata-version', '3.0');

    return this.context.httpClient.post(
    `${this.context.pageContext.web.absoluteUrl}/_api/SP.UserProfiles.PeopleManager/SetSingleValueProfileProperty`,
      {
        body: JSON.stringify(postBody),
        headers: reqHeaders
      })
    .then((response: any) => {
        return response.json();
    });
@wictorwilen
Contributor

Yes, we know that is a workaround. Or just adding an empty string as value.

@patmill
Contributor
patmill commented Aug 22, 2016

At the same time, we should follow up with the search end points to support OData v4 as well. Consistency and all that.

@wictorwilen
Contributor

Which one do you think is fastest, you fixing a few lines of TypeScript or a DCR for SharePoint ๐Ÿ˜œ

@patmill
Contributor
patmill commented Aug 22, 2016 edited

So cruel.

Also - a discussion is going on here as well for people that are hitting this. http://sharepoint.stackexchange.com/questions/191340/how-to-supply-a-querytext-parameter-to-the-search-rest-api-without-receiving-an

@patmill patmill added the enhancement label Aug 22, 2016
@pgonzal
pgonzal commented Aug 23, 2016 edited

At present, this behavior is "by design", and the workaround you suggested is the intended usage.

We have been considering a design change where all the HttpClient "magic" would be opt-in, e.g. via an enum bitfield maybe with an HttpClientOptions.DefaultsV1 that captures the current set of features. This would serve several purposes:

  1. More explicitly document the magic features that are happening behind the scenes
  2. Give us the ability to introduce new features in the future without breaking existing code (e.g. by adding a DefaultsV2 constant)
  3. Give developers an easier way to opt-out, e.g. by defining their own bitfield constants

Your example above would change to look something like this (pseudocode):

enum HttpClientFeatures {
  FetchRequestDigest = 1<<0,
  ODataVersion4 = 1<<1,
  AcceptHeader = 1<<2,
  ContentTypeHeader = 1<<3,
  DefaultsV1 = FetchRequestDigest|ODataVersion4|AcceptHeader|ContentTypeHeader
}

const httpClientFeaturesForSearch = HttpClientFeatures.DefaultsV1 & ~HttpClientFeatures.ODataVersion4;

return this.context.httpClient.post(
    `${this.context.pageContext.web.absoluteUrl}/_api/SP.UserProfiles.PeopleManager/SetSingleValueProfileProperty`,
      {
        features: httpClientFeaturesForSearch,
        headers: {
          'OData-Version': '3.0'          
        },
        body: JSON.stringify(postBody)
      })
    .then((response: any) => {
        return response.json();
    });

"Opt-in" would mean that if you omit the "features:" parameter, then you would get the vanilla BasicHttpClient behavior.

What would you think of this approach?

@pgonzal pgonzal changed the title from The HttpClient adds a "odata-version" header to all requests to Consider making HttpClient features opt-in Aug 23, 2016
@pgonzal pgonzal added this to the Undecided milestone Aug 23, 2016
@pgonzal
pgonzal commented Aug 23, 2016

VSO 239957

@wictorwilen
Contributor

@pgonzal: I like it!
I like it a lot

@sebastianrogers

As a user my feelings are that:

  • If its 'magic' then it should always work and explicitly passing an 'empty' parameter to get around it feels odd.
  • If there is an 'opt-in' for the magic then I would want it on by default.
  • I want control at a lower level than my web part, so for some calls I might want the magic and for some I might not.

Therefore I'd ideally like:

  • An 'opt-out' setting I can pass to each function call, i.e. 'no-magic' or 'magic-version-x' with the default being 'magic-version-latest'.
  • The parameter to be mandatory so I have to make an informed choice each time.

Just my tuppennies worth.

Sebastian

@pgonzal pgonzal added the Tracked label Aug 23, 2016
@pgonzal pgonzal removed this from the Undecided milestone Aug 23, 2016
@pgonzal
pgonzal commented Nov 4, 2016 edited

Following up: It turns out we can't use an enum because of our class hierarchy (enums cannot be inherited). Instead, we're going to introduce a "config" object which conforms to an interface like this:

NOTE: We are in the process of renaming HttpClient -> SPHttpClient

/**
 * The ISPHttpClientConfig object provides a set of switches for enabling/disabling
 * various features of the SPHttpClient class.  Normally these switches are set
 * (e.g. when calling SPHttpClient.fetch()) by providing one of the predefined defaults
 * from SPHttpClientConfigs, however switches can also be changed via the
 * ISPHttpClientOptions.configOverrides option.
 * @alpha
 */
export interface ISPHttpClientConfig extends IHttpClientConfig {
  /**
   * When this switch is true:
   * If RequestInit.credentials is not explicitly specified for the request,
   * then SPHttpClient will assign it to be 'same-origin'.  Without this switch,
   * different web browsers may apply different defaults.
   *
   * For more information, see the spec:
   * https://fetch.spec.whatwg.org/#cors-protocol-and-credentials
   */
  defaultSameOriginCredentials?: boolean;

  /**
   * When this switch is specified (i.e. not undefined):
   * If the 'OData-Version' header was not explictly added for the request,
   * then SPHttpClient will add the header to specify the version indicated
   * by defaultODataVersion.
   *
   * NOTE: Without an 'OData-Version' header, the SharePoint server currently
   * defaults to Version 3.0 in most cases.  The recommended version is 4.0.
   */
  defaultODataVersion?: ODataVersion;

  /**
   * When this switch is true:
   * If the 'X-RequestDigest' header was not explicitly added for the request,
   * then SPHttpClient will add it if the request is a write operation (i.e.
   * an HTTP method other than 'GET', 'HEAD', or 'OPTIONS').  The request digest
   * is managed by the DigestCache service.  In the case of a cache miss, an
   * additional network request may be performed.
   */
  fetchRequestDigest?: boolean;

  /**
   * When this switch is true:
   * If the 'Content-Type' header was not explicitly added for the request,
   * then SPHttpClient will add it if the request is a write operation (i.e.
   * an HTTP method other than 'GET', 'HEAD', or 'OPTIONS').
   * For OData 3.0, the value is 'application/json;odata=verbose;charset=utf-8'.
   * For OData 4.0, the value is 'application/json;charset=utf-8'.
   */
  jsonRequest?: boolean;

  /**
   * When this switch is true:
   * If the 'Accept' header was not explicitly added for the request,
   * then SPHttpClient will add it.
   * For OData 3.0, the value is 'application/json'.
   * For OData 4.0, the value is 'application/json;odata.metadata=minimal'.
   */
  jsonResponse?: boolean;
}

/**
 * This class provides standard predefined ISPHttpClientConfig objects for use with
 * the SPHttpClient class.  In general, clients should choose the latest available
 * version number, which enables all the switches that are recommended for typical
 * scenarios.  (If new switches are introduced in the future, a new version number
 * will be introduced, which ensures that existing code will continue to function the
 * way it did at the time when it was tested.)
 * @alpha
 */
export default class SPHttpClientConfigs {
  /**
   * This config turns off every feature switch for HttpClient.  The fetch()
   * behavior will be essentially identical to the WHATWG standard API that
   * is documented here:
   * https://fetch.spec.whatwg.org/
   */
  public static none: IHttpClientConfig = lodash.merge({},
    HttpClientConfigs.none,
    {
      defaultSameOriginCredentials: false,
      defaultODataVersion: undefined,
      fetchRequestDigest: false,
      jsonRequest: false,
      jsonResponse: false
    }
  );

  /**
   * Version 1 enables these switches:
   * consoleLogging=true;
   * magic: true
   */
  public static v1: ISPHttpClientConfig = lodash.merge({},
    HttpClientConfigs.v1,
    {
      defaultSameOriginCredentials: true,
      defaultODataVersion: ODataVersion.v4,
      fetchRequestDigest: true,
      jsonRequest: true,
      jsonResponse: true
    }
  );
}

The usage would be like this:

public deletePage(listItemId: number): Promise<void> {
  const requestUrl: string = combineURLPaths(this._webServerRelativeUrl,
    `/_api/web/lists/GetByTitle('${this._pageContext.list.title}')`,
    `/items(${listItemId})`);

  // Extra parameter is "SPHttpClientConfigs.v1"
  return this._spHttpClient.post(requestUrl, SPHttpClientConfigs.v1, {
    headers: {
      'If-Match': '*',
      'X-HTTP-Method': 'DELETE'
    }
  }).then((response: Response) => {
    //...
    return;
  });
}

In other words, every call to fetch()/post()/get() will have a mandatory additional parameter that indicates the set of "magic" at the time of writing, generally described as a standard version defined by Microsoft. This means e.g. after we're on SPHttpClientConfigs.v4, you can still copy+paste code from the internet that was written with v1, and it will work as it did at the time when it was written.

This design also permits you to define your own config (i.e. set of switches), but I think this is a very rare scenario.

A more common case is that you want to disable certain switches for a particular request. This will be supported via a "configOverrides" parameter like this:

/**
 * This interface defines the options for the SPHttpClient operations such as
 * get(), post(), fetch(), etc.  It is based on the WHATWG API standard
 * parameters that are documented here:
 * https://fetch.spec.whatwg.org/
 * @alpha
 */
export interface ISPHttpClientOptions extends IHttpClientOptions {
  /**
   * Individual switches that are set in this configOverrides property will
   * supercede the 'config' parameter for functions such as SPHttpClient.fetch().
   */
  configOverrides?: ISPHttpClientConfig;

  /**
   * For a write operation, SPHttpClient will automatically add the
   * "X-RequestDigest" header, which may need to be fetched using a seperate
   * request such as "https://example.com/sites/sample/_api/contextinfo".
   * Typically the SPWeb URL ("https://example.com/sites/sample" in this
   * example) can be guessed by looking for a reserved URL segment such
   * as "_api" in the original REST query, however certain REST endpoints
   * do not contain a reserved URL segment; in this case, the webUrl can
   * be explicitly specified using this option.
   */
  webUrl?: string;
}

...which would be used like this:

public deletePage(listItemId: number): Promise<void> {
  const requestUrl: string = combineURLPaths(this._webServerRelativeUrl,
    `/_api/web/lists/GetByTitle('${this._pageContext.list.title}')`,
    `/items(${listItemId})`);

  // Extra parameter is "SPHttpClientConfigs.v1"
  return this._spHttpClient.post(requestUrl, SPHttpClientConfigs.v1, {
    // Override  "SPHttpClientConfigs.v1" to specify jsonResponse=false
    configOverrides: {
      jsonResponse: false
    },
    headers: {
      'If-Match': '*',
      'X-HTTP-Method': 'DELETE'
    }
  }).then((response: Response) => {
    //...
    return;
  });
}

This is the "opt-out" that @sebastianrogers asked for. It says "give me everything in v1, whatever that is, minus this one thing."

Does this design make sense? To summarize, these are the three problems with the existing design that we are seeking to solve:

  1. The magic is mysterious; devs are surprised to find headers being injected behind the scenes, and then they wonder what other magic is happening. The rules are difficult to document because they don't have names.
  2. No easy "opt out": Sometimes devs don't want the magic. There are ways to disable most of it (e.g. by explicitly specifying the header) but this is awkward and difficult to document.
  3. Backwards compatibility: If we introduce new magic in the future, it may break existing code (e.g. that assumed the header would NOT be added). We could solve this by forking a new major version of the framework package, but that is very expensive.
@lijunle
lijunle commented Nov 26, 2016

It says "give me everything in v1, whatever that is, minus this one thing."

To archieve this, why not use Object.assign() API to merge two option? Use a separated configOverrides object seems strange to me.

@pgonzal
pgonzal commented Nov 29, 2016

We ended up making the configuration into an actual TypeScript class. This also solves a technical problem where an options object could accidentally be passed as the config (since any object satisfies an interface whose properties are all optional).

The current draft looks like this:

  spHttpClient.get(
    'http://example.com/_api/test01',
    /* config: */ SPHttpClientConfigurations.v1.overrideWith({
      defaultODataVersion: ODataVersion.v3, // "OData-Version" will be 3.0
      jsonResponse: false                   // "Accept" header should NOT be added
    }),
    /* options: */ {
      webUrl: 'http://example.com/'
    }
  ).then(() => {
    ...
  })
@patmill
Contributor
patmill commented Feb 24, 2017

Cleaning up resolved bugs.

@patmill patmill closed this Feb 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment