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

feat(kv): label names to be unique #15647

Merged
merged 8 commits into from
Oct 31, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
1. [15556](https://github.com/influxdata/influxdb/pull/15556): Creating a check now displays on the checklist
1. [15592](https://github.com/influxdata/influxdb/pull/15592): Changed task runs success status code from 200 to 201 to match Swagger documentation.
1. [15634](https://github.com/influxdata/influxdb/pull/15634): TextAreas have the correct height
1. [15647](https://github.com/influxdata/influxdb/pull/15647): Ensures labels are unique by organization in the kv store

## v2.0.0-alpha.18 [2019-09-26]

Expand Down
129 changes: 127 additions & 2 deletions kv/label.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"bytes"
"context"
"encoding/json"
"fmt"
"strings"

"github.com/influxdata/influxdb"
"github.com/influxdata/influxdb/kit/tracing"
Expand All @@ -12,6 +14,7 @@ import (
var (
labelBucket = []byte("labelsv1")
labelMappingBucket = []byte("labelmappingsv1")
labelIndex = []byte("labelindexv1")
)

func (s *Service) initializeLabels(ctx context.Context, tx Tx) error {
Expand All @@ -23,6 +26,10 @@ func (s *Service) initializeLabels(ctx context.Context, tx Tx) error {
return err
}

if _, err := tx.Bucket(labelIndex); err != nil {
return err
}

return nil
}

Expand Down Expand Up @@ -85,7 +92,7 @@ func (s *Service) findLabelByID(ctx context.Context, tx Tx, id influxdb.ID) (*in

func filterLabelsFn(filter influxdb.LabelFilter) func(l *influxdb.Label) bool {
return func(label *influxdb.Label) bool {
return (filter.Name == "" || (filter.Name == label.Name)) &&
return (filter.Name == "" || (strings.EqualFold(filter.Name, label.Name))) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

is this change to maintain this with utf8 chars? what's the motivation behind this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah. our CI linter also seemed to agree with the change.

I had changed things originally to deal w/ finding labels case insensitively like so
strings.ToLower(filter.Name) == strings.ToLower(label.Name) and CI called foul on that use.

((filter.OrgID == nil) || (filter.OrgID != nil && *filter.OrgID == label.OrgID))
}
}
Expand Down Expand Up @@ -248,6 +255,19 @@ func (s *Service) deleteLabelMapping(ctx context.Context, tx Tx, m *influxdb.Lab
// CreateLabel creates a new label.
func (s *Service) CreateLabel(ctx context.Context, l *influxdb.Label) error {
err := s.kv.Update(ctx, func(tx Tx) error {
if err := l.Validate(); err != nil {
return &influxdb.Error{
Code: influxdb.EInvalid,
Err: err,
}
}

l.Name = strings.TrimSpace(l.Name)

if err := s.uniqueLabelName(ctx, tx, l); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

one thing we may want to do here is move the read for unqiueness outside the update. In cloud this would force a TX imagine, which seems like a bad idea for this. Moving this to a separate read before the update seems like a better idea 🤔. Also, why are we trimming white space on line 265? is that a behavior that we share across the kv implementations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re: transactions - I'm just leaving a note here since we chatted about this. This is an even larger issue that may not get addressed in this PR. TX is pretty heavily depended on unfortunately for reading/writing to kv buckets.

return err
}

l.ID = s.IDGenerator.ID()

if err := s.putLabel(ctx, tx, l); err != nil {
Expand Down Expand Up @@ -395,7 +415,34 @@ func (s *Service) updateLabel(ctx context.Context, tx Tx, id influxdb.ID, upd in
}

if upd.Name != "" {
upd.Name = strings.TrimSpace(upd.Name)

idx, err := tx.Bucket(labelIndex)
if err != nil {
return nil, &influxdb.Error{
Err: err,
}
}

key, err := labelIndexKey(label)
if err != nil {
return nil, &influxdb.Error{
Err: err,
}
}

if err := idx.Delete(key); err != nil {
return nil, &influxdb.Error{
Err: err,
}
}

label.Name = upd.Name
if err := s.uniqueLabelName(ctx, tx, label); err != nil {
return nil, &influxdb.Error{
Err: err,
}
}
}

if err := label.Validate(); err != nil {
Expand Down Expand Up @@ -430,6 +477,26 @@ func (s *Service) putLabel(ctx context.Context, tx Tx, l *influxdb.Label) error
}
}

idx, err := tx.Bucket(labelIndex)
if err != nil {
return &influxdb.Error{
Err: err,
}
}

key, err := labelIndexKey(l)
if err != nil {
return &influxdb.Error{
Err: err,
}
}

if err := idx.Put([]byte(key), encodedID); err != nil {
return &influxdb.Error{
Err: err,
}
}

b, err := tx.Bucket(labelBucket)
if err != nil {
return err
Expand Down Expand Up @@ -499,7 +566,7 @@ func (s *Service) DeleteLabel(ctx context.Context, id influxdb.ID) error {
}

func (s *Service) deleteLabel(ctx context.Context, tx Tx, id influxdb.ID) error {
_, err := s.findLabelByID(ctx, tx, id)
label, err := s.findLabelByID(ctx, tx, id)
if err != nil {
return err
}
Expand All @@ -521,6 +588,26 @@ func (s *Service) deleteLabel(ctx context.Context, tx Tx, id influxdb.ID) error
}
}

idx, err := tx.Bucket(labelIndex)
if err != nil {
return &influxdb.Error{
Err: err,
}
}

key, err := labelIndexKey(label)
if err != nil {
return &influxdb.Error{
Err: err,
}
}

if err := idx.Delete(key); err != nil {
return &influxdb.Error{
Err: err,
}
}

if err := s.deleteUserResourceMappings(ctx, tx, influxdb.UserResourceMappingFilter{
ResourceID: id,
ResourceType: influxdb.LabelsResourceType,
Expand All @@ -530,3 +617,41 @@ func (s *Service) deleteLabel(ctx context.Context, tx Tx, id influxdb.ID) error

return nil
}

// labelAlreadyExistsError is used when creating a new label with
// a name that has already been used. Label names must be unique.
func labelAlreadyExistsError(lbl *influxdb.Label) error {
return &influxdb.Error{
Code: influxdb.EConflict,
Msg: fmt.Sprintf("label with name %s already exists", lbl.Name),
}
}

func labelIndexKey(l *influxdb.Label) ([]byte, error) {
orgID, err := l.OrgID.Encode()
if err != nil {
return nil, &influxdb.Error{
Code: influxdb.EInvalid,
Err: err,
}
}

k := make([]byte, influxdb.IDLength+len(l.Name))
copy(k, orgID)
copy(k[influxdb.IDLength:], []byte(strings.ToLower((l.Name))))
Copy link
Contributor

Choose a reason for hiding this comment

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

are we making names unique case insensitive now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. label names should be unique & case agnostic. the UI currently handles all this stuff. feels like it's eagerly loading the labels and doing the label name validation in the browser...

for this change I added, the casing of the label names in the label bucket shouldn't matter & can be a mix of upper and lower case no problem. this system bucket is going to store the down-cased version of the label name, and we'll be using that to decide if a matching label name string already exists.

Copy link
Contributor

Choose a reason for hiding this comment

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

excellent! still feels like a service concern, not necessarily a store concern 🤷‍♂, not even sure where that fits in here tbh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed

return k, nil
}

func (s *Service) uniqueLabelName(ctx context.Context, tx Tx, lbl *influxdb.Label) error {
key, err := labelIndexKey(lbl)
if err != nil {
return err
}

// labels are unique by `organization:label_name`
err = s.unique(ctx, tx, labelIndex, key)
if err == NotUniqueError {
return labelAlreadyExistsError(lbl)
}
return err
}
9 changes: 9 additions & 0 deletions label.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,15 @@ const (
OpDeleteLabelMapping = "DeleteLabelMapping"
)

// errors on label
var (
// ErrLabelNameisEmpty is error when org name is empty
ErrLabelNameisEmpty = &Error{
Code: EInvalid,
Msg: "label name is empty",
}
)

// LabelService represents a service for managing resource labels
type LabelService interface {
// FindLabelByID a single label by ID.
Expand Down
4 changes: 2 additions & 2 deletions testing/document.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ func NewDocumentIntegrationTest(store kv.Store) func(t *testing.T) {
t.Fatalf("failed to find document store: %v", err)
}

l1 := &influxdb.Label{Name: "l1"}
l2 := &influxdb.Label{Name: "l2"}
l1 := &influxdb.Label{Name: "l1", OrgID: MustIDBase16("41a9f7288d4e2d64")}
l2 := &influxdb.Label{Name: "l2", OrgID: MustIDBase16("41a9f7288d4e2d64")}
mustCreateLabels(ctx, svc, l1, l2)
lBad := &influxdb.Label{ID: MustIDBase16(oneID), Name: "bad"}

Expand Down
Loading