[Security Solution][Cases] Update messages storage

[michaelolo24]

Summary

This PR re-introduces the message storage for the callout component in cases with a singleton in place of the storage service implementation currently in the security solution. The service implementation is not needed in security as the storage is only used in one location within cases.

Checklist

Delete any items that are not applicable to this PR.

• Unit or functional tests were updated or added to match the most common scenarios

[elasticmachine]

Pinging @elastic/security-solution (Team: SecuritySolution)

[elasticmachine]

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

[oatkiller]

Thanks for working on this refactor

[oatkiller]

I think this line won't work as intended since the state is kept in a module level binding: let instance: MessagesStorage;

[michaelolo24]

It should as MessagesStorage doesn't maintain its own state. It's just a wrapper around localStorage where the state is stored. Maybe I'm missing something?

[oatkiller]

Perhaps you could instead model this using a react provider and context. The provider would be used at the root of the react app and it could create an instance of MessageStorage. With that design, there would be no need for a module level binding here. The advantage is that each react app (i.e. each one created via a React.mount call) can create its own instance of MessagesStorage and each automated test call can create its own instance of MessagesStorage as well.

This also removes a side effect from the react app. Instead it allows the code that owns the react app to create and maintain a binding to the MessagesStorage.

[michaelolo24]

Yea, that's much cleaner and will avoid any potential cross test pollution. None of these component/views are going to be directly mounted as top level views as all the plugin is doing atm is exporting view components for other apps to consume, but def a better model and more future proof.

[michaelolo24]

Actually, thinking about it some more the module level binding may be better for now. Since each view component is consumed separately, if I were to wrap each view that uses this context in its own provider, then a single application consuming multiple case views will end up having multiple instances of storage in its subtree. We could have the consuming app provide their own instance of storage? That being said, it really won't matter as the underlying key ${plugin}-messages will stay the same so all instances will end up acting on the same data 🤷🏾

[oatkiller]

I see now. I didn't realize that the singleton had no state of its own. If that's the case, there is no need to make it a singleton at all. Callers can reinstate it on each use as it has no state. In fact, it doesn't need to be a class (or object) since there is no state. You can model the code as exported functions. These functions can access storage via a global binding, or you can have the functions take a reference to a storage object. If they receive a storage object, then you can test them by passing a mock storage object.

[cnasikas]

@michaelolo24 I agree with @oatkiller. I think functions will be a better approach. I guess you didn't use const { storage } = useKibana().services; because some plugins will not provide that to the Kibana context, right? If that the reason you can use the same structure (useMessagesStorage) but instead of getting the storage from useKibana().services you can initiated yourself inside the hook. I don't see a problem with having multiple instance of the storage class. At the end, the browser's localStorage is probably a singleton.

If you really want to have one instance of the Storage and not having to require it from the consumers you can pass it to your CasesUiClient from the plugin.ts. Example:

interface CasesUiClientDeps {
  storage: Storage
}

const createCasesUiClient = ({ storage }: CasesUiClientDeps) => ({
      getAllCases: (props) => {
        return getAllCasesLazy(props, { storage });
      },
      getCaseView: (props) => {
        return getCaseViewLazy(props, { storage });
      },
      getConfigureCases: (props) => {
        return getConfigureCasesLazy(props, { storage });
      },
      getCreateCase: (props) => {
        return getCreateCaseLazy(props, { storage });
      },
      getRecentCases: (props) => {
        return getRecentCasesLazy(props, { storage });
      },
      getAllCasesSelectorModal: (props) => {
        return getAllCasesSelectorModalLazy(props, { storage });
      },
})

public start(core: CoreStart, plugins: StartPlugins): CasesUiStart {
    KibanaServices.init({ ...core, ...plugins, kibanaVersion: this.kibanaVersion });
    const storage = new Storage(localStorage);
    return createCasesUiClient({ storage });
  }

[michaelolo24]

Thanks for the feedback @oatkiller and @cnasikas. After getting caught up in making these minor changes, I realized that the code that was there already is all I actually wanted 😂. This PR was initially made because we weren't sure if we would still need the singleton/service setup and then I went down a completely unnecessary rabbit hole 😂. Thanks for the feedback though! Gonna close this PR

[michaelolo24]

@cnasikas - one thing this does bring up then is that we probably don't need to be passing storage as a service in security_solution either..

[michaelolo24]

Unnecessary changes. Storage uses localStorage under the hood so no need to worry about multiple instances...

[kibanamachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: 8901491
• Storybooks not built
• Pipeline Steps (look for red circles / failed steps)
• Interpreting CI Failures

Failed CI Steps

• Jest Unit Tests
• Build X-Pack Plugins
• Report failed tests, if necessary

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [76ae331]

History

• 💚 Build #120716 succeeded 4fe24ee

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream