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

Enforce naming rules on indexes and collections #1531

Merged
merged 6 commits into from
Nov 28, 2019

Conversation

scottinet
Copy link
Contributor

Description

Enforce strict naming rules on indexes and collections. This has the following effects:

  • index and collection creation now return dedicated error messages/ids (index:create and collection:create)
  • it's no longer possible to restrict a profile to an index or a collection that cannot possibly exist (opened issue: KZL-1392)

The enforced rules are a stricter version of the Elasticsearch naming rules, as Kuzzle adds a few restrictions of its own.

The documentation has also been updated with these naming rules.

Architecture

Naming rules have been implemented directly in the ES service, and propagated through the ClientAdapter class. I've made that choice because, to me, naming rules depend on the implemented storage service and therefore might change depending on it.

That way, if we use another storage service, it simply needs to implement the new isIndexNameValid and isCollectionNameValid.

This choice also explains why we have 1 dedicated function per object type (index & collection) even if this is useless with ES since we emulate collections with indexes. This paves the way for other storage engines allowing different structures and maybe different naming rules for indexes and collections.

@scottinet scottinet added the 2.x label Nov 26, 2019
@codecov
Copy link

codecov bot commented Nov 26, 2019

Codecov Report

Merging #1531 into 2-dev will increase coverage by 0.03%.
The diff coverage is 96.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##            2-dev    #1531      +/-   ##
==========================================
+ Coverage   93.49%   93.52%   +0.03%     
==========================================
  Files          98       98              
  Lines        6729     6749      +20     
==========================================
+ Hits         6291     6312      +21     
+ Misses        438      437       -1
Impacted Files Coverage Δ
lib/core/storage/clientAdapter.js 95.55% <ø> (ø) ⬆️
lib/core/models/security/profile.js 97.46% <100%> (+0.17%) ⬆️
lib/core/models/repositories/profileRepository.js 98.86% <100%> (+0.01%) ⬆️
lib/api/controllers/collection.js 89.89% <100%> (ø) ⬆️
lib/services/elasticsearch.js 90.47% <93.75%> (+0.45%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c25a4c...6013ffb. Read the comment docs.

lib/services/elasticsearch.js Outdated Show resolved Hide resolved
}

let valid = true;
const forbiddenChars = `\\/*?"<>| \t\r\n,#:${NAME_SEPARATOR}${PUBLIC_PREFIX}${INTERNAL_PREFIX}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

(Nit): Can't we declare the forbidden chars once time only?

lib/services/elasticsearch.js Outdated Show resolved Hide resolved
@kuzzle
Copy link
Contributor

kuzzle commented Nov 28, 2019

SonarQube analysis reported 3 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 elasticsearch.js#L1682: Complete the task associated to this TODO comment. rule
  2. INFO elasticsearch.js#L1736: Complete the task associated to this TODO comment. rule
  3. INFO elasticsearch.js#L1816: Complete the task associated to this TODO comment. rule

@scottinet scottinet merged commit b6443aa into 2-dev Nov 28, 2019
@scottinet scottinet deleted the KZL-1392/precheck-index-names branch November 28, 2019 09:01
@scottinet scottinet mentioned this pull request Dec 6, 2019
scottinet added a commit that referenced this pull request Dec 6, 2019
# [2.0.1](https://github.com/kuzzleio/kuzzle/releases/tag/2.0.1) (2019-12-06)


#### Bug fixes

- [ [#1534](#1534) ] Configured server limit were not applied to some m* and deleteByQuery routes   ([scottinet](https://github.com/scottinet))
- [ [#1539](#1539) ] Fix URL parsing w/ trailing slash & querystring   ([scottinet](https://github.com/scottinet))
- [ [#1531](#1531) ] Enforce naming rules on indexes and collections   ([scottinet](https://github.com/scottinet))
- [ [#1530](#1530) ] Rejects new API requests during a shutdown sequence   ([scottinet](https://github.com/scottinet))
- [ [#1504](#1504) ] Do not delete a profile still assigned to users   ([Yoann-Abbes](https://github.com/Yoann-Abbes))

#### Enhancements

- [ [#1533](#1533) ] Add plugin version to server info   ([Aschen](https://github.com/Aschen))
- [ [#1536](#1536) ] Prevent overloads caused by pre-funnel pipes   ([scottinet](https://github.com/scottinet))
- [ [#1543](#1543) ] Use [email protected]   ([scottinet](https://github.com/scottinet))
---
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants