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

Refactor controllers #1446

Open
wants to merge 196 commits into
base: 2-dev
from

Conversation

@Aschen
Copy link
Contributor

commented Sep 3, 2019

What does this PR do ?

Disclaimer: I know there is a lot of things that could be improved (eg: stop using assert in security controller) but this PR is big enough. I left a lot of @todo that we could handle later

Storage

Kuzzle storage is now handled by the StorageEngine class.
This class instantiate 2 scoped Elasticsearch services wrapped in ClientAdapter, one for public and one for internal indexes.
It also contain the RAM indexCache and associated methods.

Elasticsearch services are wrapped by the ClientAdapter class. This class maintain the indexCache and test for index/collection existence before calling ES service methods. Controllers and core components (eg: internalIndex) are no longer responsible for the IndexCache.

The IndexStorage class is a wrapper around an ES index. This class has roughly the same methods as the ES service but they are all scoped to an index. It is used for the internal index and also plugin indexes.

Cache

Now the internalCache and memoryStorage services are instantiated in the CacheEngine class. Also, they are renamed to public and internal:

kuzzle.cacheEngine.internal
kuzzle.cacheEngine.public

Controllers

Helpers methods have been added to the BaseController class. These methods are used to extract param from the request object instead of asserting there presence and then get them.

Index Controller

  • Breaking change Remove following methods: getAutoRefresh, refresh, refreshInternal, setAutoRefresh

Collection Controller

  • New feature Add refresh method
Aschen and others added 30 commits Aug 8, 2019
wip
wip
nit
wip
Aschen and others added 8 commits Sep 15, 2019
[fix] fix an unhandled promise rejection (#1464)
Most API routes in the admin controller can respond immediately if the refresh flag is not set. But in this case, the underlying promises created are unhandled if they are rejected, which makes Kuzzle crash in development environments.
Merge branch 'KZL-1302/refactor-controllers' of github.com:kuzzleio/k…
…uzzle into KZL-1302/refactor-controllers

@Aschen Aschen removed the wip label Sep 16, 2019

/**
* Reset the internal storage components (internal index, cache and public storage)
*/
resetKuzzleData (request) {

This comment has been minimized.

Copy link
@Aschen

Aschen Sep 16, 2019

Author Contributor

Remove this method since it can lead to inconsistency with the JWT seed in a cluster environment

@@ -92,6 +99,248 @@ class BaseController {

return Boolean(flagValue);
}

This comment has been minimized.

Copy link
@Aschen

Aschen Sep 16, 2019

Author Contributor

New methods to assert param presence and extract them from request object

@@ -0,0 +1,57 @@
/*

This comment has been minimized.

Copy link
@Aschen

Aschen Sep 16, 2019

Author Contributor

New CacheEngine

@@ -0,0 +1,161 @@
/*

This comment has been minimized.

Copy link
@Aschen

Aschen Sep 16, 2019

Author Contributor

New ClientAdapter class which is in charge to maintain the index cache and check index/collection existence

@@ -0,0 +1,145 @@
/*

This comment has been minimized.

Copy link
@Aschen

Aschen Sep 16, 2019

Author Contributor

This class is a wrapper around an index and a storageEngine. It provide the same capabilities than a storage engine but restricted to a specific index.
Renamed from IndexEngine and use metaprogrammation instead of redeclaring each method

@@ -22,67 +22,71 @@
'use strict';

const
assert = require('assert'),
errorsManager = require('../../../config/error-codes/throw'),
Elasticsearch = require('../../../services/elasticsearch'),

This comment has been minimized.

Copy link
@Aschen

Aschen Sep 16, 2019

Author Contributor

New StorageEngine class who instantiate the public and internal storage engines through ClientAdapter class.
This class also keep the RAM index cache

if (position !== -1) {
this._indexes[index].collections.splice(position, 1);

if (this._indexes[index].collections.length === 0) {

This comment has been minimized.

Copy link
@Aschen

Aschen Sep 16, 2019

Author Contributor

Since indexes are just volatile collection containers, we delete them from the cache when there is no collection left inside

Aschen added 4 commits Sep 16, 2019
nit
nit
@kuzzle

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

SonarQube analysis reported 12 issues

  • INFO 12 info

Watch the comments in this conversation to review them.

3 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. INFO tokenRepository.js#L224: Complete the task associated to this TODO comment. rule
  2. INFO pluginContext.js#L96: Complete the task associated to this TODO comment. rule
  3. INFO kuzzle.js#L54: Complete the task associated to this TODO comment. rule

---

## Response

Returns an object detailing the status of the forced refresh.
Returns a response with `status` 200 if refresh succeed.

This comment has been minimized.

Copy link
@scottinet

scottinet Sep 17, 2019

Member
Suggested change
Returns a response with `status` 200 if refresh succeed.
Returns a response with `status` 200 if the refresh succeeds.

If one or more document retrievals fail, the response status is set to `206`, and the `error` object contain a [partial error](/core/2/api/essentials/errors#partialerror) error.
If one or more document retrievals fail, the response status is set to `206`, and the `error` object contain a [partial error](/core/1/api/essentials/errors#partialerror) error.

This comment has been minimized.

Copy link
@scottinet

scottinet Sep 17, 2019

Member
Suggested change
If one or more document retrievals fail, the response status is set to `206`, and the `error` object contain a [partial error](/core/1/api/essentials/errors#partialerror) error.
If one or more document retrievals fail, the response status is set to `206`, and the `error` object contains a [partial error](/core/1/api/essentials/errors#partialerror) error.
If one or more document retrievals fail, the response status is set to `206`, and the `error` object contain a [partial error](/core/1/api/essentials/errors#partialerror) error.

::: info
You can use the `found` attribute on hits to identify missing documents.

This comment has been minimized.

Copy link
@scottinet

scottinet Sep 17, 2019

Member
Suggested change
You can use the `found` attribute on hits to identify missing documents.
You can use the `found` attribute to identify missing documents.

**Bulk Controller**

- `bulk:import`: it's no longer allowed to specify different index and collection in bulk data array

This comment has been minimized.

Copy link
@scottinet

scottinet Sep 17, 2019

Member
Suggested change
- `bulk:import`: it's no longer allowed to specify different index and collection in bulk data array
- `bulk:import`: it's no longer allowed to specify different indexes and collections in the same bulk data array

**Internal datamodel changes:**

- `kuzzle` index and its collections changes according to our new naming policy

This comment has been minimized.

Copy link
@scottinet

scottinet Sep 17, 2019

Member
Suggested change
- `kuzzle` index and its collections changes according to our new naming policy
- `kuzzle` index and its collections now follow our new naming policy
_bootstrapSequence (...args) {
throw new Error(`To be implemented. args: ${args.join()}`);
async _bootstrapSequence () {
// can be overrided

This comment has been minimized.

Copy link
@scottinet

scottinet Sep 17, 2019

Member
Suggested change
// can be overrided
// can be overridden
'listCollections'
];

// Methods directly bind to the storageClient

This comment has been minimized.

Copy link
@scottinet

scottinet Sep 17, 2019

Member
Suggested change
// Methods directly bind to the storageClient
// Methods directly bound to the storageClient

// Methods that need to assert index/collection existence and add/remove item
// from the indexCache
this._othersMethods = [

This comment has been minimized.

Copy link
@scottinet

scottinet Sep 17, 2019

Member

Poor object name, since this appears to be the most important collection of methods.

What about this._cacheChangeMethods?

deleteIndex (index) {
this._assertIndexExists(index);

return this._client.deleteIndex(index);

This comment has been minimized.

Copy link
@scottinet

scottinet Sep 17, 2019

Member

shouldn't we remove that index from the cache if the clients successfully deletes it?

this._assertIndexExists(index);
}

return this._client.deleteIndexes(indexes);

This comment has been minimized.

Copy link
@scottinet

scottinet Sep 17, 2019

Member

shouldn't we remove those indexes from the cache if the clients successfully deletes them?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.