[state] store actual state value in session storage

[spalger]

Fixes: #3947

Approach

This solution focuses on the way that the appState and globalState communicate with the URL. By intercepting all reads/writes to the $location service in the State classes (superclass for both app and global state) we can store a hash of the content in the URL, and the actual content in sessionStorage.

The primary mechanisms providing this functionality are the HashedItemStore, the createStateHash() function, and the isStateHash() function.

The createStateHash() function accepts a string and produces the short hashes shown in the url.

The isStateHash() function is a identifies the hashes produced by the createStateHash() function by their h@ prefix. This is how we accomplish backwards-compatibility.

The HashedItemStore is essentially a least-recently-used cache built on top of session storage. It's primary responsibility is tracking which hashes are used and when so that when no more history entries will fit in sessionStorage the oldest entries can be removed until enough space is available.

Backwards Compatability

Since the hashes produced by createStateHash() can be identified by the isStateHash() function, the State class is able to support state query string param values that are either hashed or rison formatted. When a rison formatted query string value is found it is decoded and immediately converted to a hash.

Opening stateful urls

Since the hash in the URL can not be converted back to it's raw state value without access to session storage (and session storage is tab specific), copying the url in the browser chrome and pasting it into a new tab will cause a (hopefully helpful) error to be shown. This error directs users to the share ui where they can get an un-hashed version of the URL or create a short url from that full un-hashed url. -- related #8051

In order to prevent requiring this step at all times, all links throughout the UI use the same un-hashed version of the url that the share ui exposes. Because these links do not have hashes they can be opened in a new tab without issue (via right-click or ctrl/cmd-click)

[spalger]

store.length Is a thing

[spalger]

Now that #8021 is merged, this is ready for review. And I'll stop rebasing until review is complete.

[cjcenizal]

I was having a hard time understanding the role of this module and what it was doing internally, so I tried renaming things in a way that told me a story. Do you think these changes below make the logic easier to follow?

Also, I think it's possible to split each of these methods into their own individual modules. It doesn't seem necessary to group them into one. I mostly came to that conclusion because I was having a hard time figuring out a better name for this file/module, and then I realized it was because these methods don't seem cohesive enough to be methods on an object.

/**
 * get_url_with_states.js
 * Convert a URL that contains state references to one that contains RISON-encoded state values.
 */
export default function getUrlWithStates(urlWithRefs) {
  if (!urlWithRefs) return;

  const urlWithRefsObject = parseUrl(urlWithRefs, true);
  if (!urlWithRefsObject.hostname) {
    // passing a url like "localhost:5601" or "/app/kibana" should be prevented
    throw new TypeError(
      'Only absolute urls should be passed to `unhashStates.inAbsUrl()`. ' +
      'Unable to detect url hostname.'
    );
  }

  if (!urlWithRefsObject.hash) return urlWithRefs;

  // e.g. /discover?_g=h@44136&_a=h@cdf09
  const clientRoute = urlWithHashesObject.hash.slice(1); // trim the #
  if (!clientRoute) return urlWithRefs;

  const clientRouteObject = parseUrl(clientRoute, true);

  // e.g. {_g: h@44136, _a: h@cdf09}
  const queryParamToRefMap = clientRouteObject.query;
  if (!queryParamToRefMap) return urlWithRefs;

  // e.g. {_g: (rison), _a: (rison)}
  const states = [getAppState(), globalState].filter(Boolean);
  const queryParamToStateMap = mapQueryParamsToStateValues(queryParamToRefMap, states);

  // Rebuild URL with new query params with RISON state values.
  const urlWithStates = formatUrl(Object.assign({}, urlWithRefsObject, {
    hash: formatUrl({
      pathname: clientRouteObject.pathname,
      query: queryParamToStateMap,
    }),
  }));

  return urlWithStates;
};

/**
 * map_query_params_to_state_values.js
 * Map query-param-keyed hash references to one that contains RISON-encoded state values.
 */
export default function mapQueryParamsToStateValues(queryParamToRefMap, states) {
  return mapValues(queryParamToRefMap, (ref, queryParam) => {
    // Pull out the state for each query param in the query.
    const state = states.find(s => queryParam === s.getQueryParamName());
    return state ? state.toRISON() : ref;
  });
};

[spalger]

I agree with most of the naming here, and the idea of splitting it into two modules works for me, but I'm not a fan of replacing "unhash" with "toState". I see issue with "unhash", but I find "toStateValues" and "withStates" to be too generic. We have a lot of state-related things in this directory specifically, even a State class, which this doesn't really relate to. I'd rather find a more meaningful alternate, but will implement the rest of the changes.

Correction: the translation actually uses instances of the State class to lookup/unhash/resolve the state references

[spalger]

I'm gravitating around "removeStateReferences" or "dereferenceStatesInUrl" or something...

[spalger]

maybe removeStateHashesFromUrl() and mapStateHashesToStateRison()

[spalger]

The more I go down this path the more confusing it seems to call things hashes everywhere except here.

[tylersmalley]

The addition of sha.js-browser doesn't make me feel warm and fuzzy, though it only has one dependency.

What are your thoughts on removing the creating of SHA's altogether and storing an integer in sessionStorage and auto-incrementing it? If we still would like to maintain the SHA-like keys, we can start it at 100000000 and toString(36) to produce a 6 character key.

[cjcenizal]

Can we leave some comments explaining what's going on from line 48 through 52?

[spalger]

What are your thoughts on removing the creating of SHA's altogether and storing an integer in sessionStorage and auto-incrementing it?

The issue I have with that is the sheer amount of churn it would create. With hashing we can know when the value is already in localStorage. I also think the auto-incrementing numbers in the URL would look a little strange, like a counter or timer or something.. Just a thought

[tylersmalley]

With hashing we can know when the value is already in localStorage.

This is actually a fair point, especially when we have a somewhat limited space.

I also think the auto-incrementing numbers in the URL would look a little strange, like a counter or timer or something.

The user wouldn't see it as auto-incrementing. We would be converting the integer into an alpha-numeric string.

For example:

var num = 100000000;
num.toString(36); // "1njchs"

parseInt("1njchs", 36); // 100000000

I think your approach is best, though I would like to see us avoid one-off npm modules. We can either copy the relevant code into the Kibana repository or use a more accepted library like murmur

[cjcenizal]

I think if we add some variables here we can add context and improve readability:

const hashedUrl = window.location.href;
const unhashedUrl = unhashStates.inAbsUrl(hashedUrl);
internals.trackPossibleSubUrl(unhashedUrl);

[epixa]

@spalger To answer a question you posed to me last week: this should be opt-in even in 5.0.

While I'm very happy to have this fix out there, and from a technical standpoint I think this is good, I don't think this is the right approach to state management in the long term. It's just the best fix we can implement at the moment. I don't want to ship a default feature to users that we know is not the experience we want in the end anyway.

But some users really need some help from Kibana itself to address their own browser requirements, so I still think it's important that we move forward with this overall, even though it's an opt-in feature.

[tylersmalley]

LGTM, though it sounds like we need to default to opt-in.

One concern I have with opt-in is how it impacts the overall user experience. An IE user who runs into this issue will be prompted with an error and somehow will need to discover the state:storeInSessionStorage option. Adding the details of that option to the error would help, but it's still not a clean fix. If IE users are a large enough segment to justify this fix, why not simply fix it by default in 5.0?

I understand that this is not the desired long-term solution, but it points us in a direction where the URL's are ephemeral and requires users to use the share functionality as opposed to sharing the URL. I see this as a huge step forward towards a long-term solution.

[epixa]

+1 to updating the error message to point people to the appropriate setting.

IE users are a small percentage of our users, but it just so happens that a lot of IE users have no other choice due to corporate requirements, which is why this is so important to fix.

I don't think pointing people to our share functionality is the right long term solution, either. We're building a web app here: people should be able to do something as fundamental as grabbing the url from their browser to link someone to the current page. I have no doubt that that is possible in Kibana, it's just a huge undertaking that we're not prepared to tackle just yet.

[tylersmalley]

Per Court's comment, let's update to include information for enabling the state:storeInSessionStorage option.

[spalger]

Alright, this had been disabled by default and the warning in that shows up before overflow, as well as the overflow page, have been updated to link users to the setting.

[epixa]

@spalger Are you going to attempt to backport this to 4.6 or do you think it's too hairy?

[spalger]

I haven't taken the time to try yet