]> git.apps.os.sepia.ceph.com Git - ceph.git/commitdiff
mgr/dashboard: RGW buckets async validator performance enhancement
authorNizamudeen A <nia@redhat.com>
Sun, 25 Apr 2021 08:47:07 +0000 (14:17 +0530)
committerNizamudeen A <nia@redhat.com>
Wed, 12 May 2021 09:20:05 +0000 (14:50 +0530)
The rgw bucket creation form has the Name field which have an async
validator. The validator calls all the bucket name and check if the
entered name is unique or not. This happens on every keystroke. So if
100 or more buckets are there, then the async validation can be real
    slow and causes misvalidations in different fields.

I changed the validation logic and did some cleanups to improve the
performance of the async validation.

Fixes: https://tracker.ceph.com/issues/50514
Signed-off-by: Nizamudeen A <nia@redhat.com>
(cherry picked from commit 005327c4e12fef8d7054894d9df021c0b3c53e19)

src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.spec.ts
src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.ts
src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-user-form/rgw-user-form.component.spec.ts
src/pybind/mgr/dashboard/frontend/src/app/shared/api/rgw-bucket.service.spec.ts
src/pybind/mgr/dashboard/frontend/src/app/shared/api/rgw-bucket.service.ts
src/pybind/mgr/dashboard/frontend/src/app/shared/forms/cd-validators.ts

index 01f4626686a1a5b50ee82889a5b4b0032be6db1e..82e1a1e47f2bfdc92662b8d03bf95801b221d98b 100644 (file)
@@ -1,12 +1,12 @@
 import { HttpClientTestingModule } from '@angular/common/http/testing';
-import { ComponentFixture, TestBed } from '@angular/core/testing';
-import { FormControl, ReactiveFormsModule } from '@angular/forms';
+import { ComponentFixture, fakeAsync, TestBed, tick } from '@angular/core/testing';
+import { ReactiveFormsModule } from '@angular/forms';
 import { Router } from '@angular/router';
 import { RouterTestingModule } from '@angular/router/testing';
 
 import _ from 'lodash';
 import { ToastrModule } from 'ngx-toastr';
-import { of as observableOf } from 'rxjs';
+import { of as observableOf, throwError } from 'rxjs';
 
 import { RgwBucketService } from '~/app/shared/api/rgw-bucket.service';
 import { RgwSiteService } from '~/app/shared/api/rgw-site.service';
@@ -55,93 +55,75 @@ describe('RgwBucketFormComponent', () => {
 
   describe('bucketNameValidator', () => {
     const testValidator = (name: string, valid: boolean) => {
-      const validatorFn = component.bucketNameValidator();
-      const ctrl = new FormControl(name);
-      ctrl.markAsDirty();
-      const validatorPromise = validatorFn(ctrl);
-      expect(validatorPromise instanceof Promise).toBeTruthy();
-      if (validatorPromise instanceof Promise) {
-        validatorPromise.then((resp) => {
-          if (valid) {
-            expect(resp).toBe(null);
-          } else {
-            expect(resp instanceof Object).toBeTruthy();
-            expect(resp.bucketNameInvalid).toBeTruthy();
-          }
-        });
+      rgwBucketServiceGetSpy.and.returnValue(throwError('foo'));
+      formHelper.setValue('bid', name, true);
+      tick(500);
+      if (valid) {
+        formHelper.expectValid('bid');
+      } else {
+        formHelper.expectError('bid', 'bucketNameInvalid');
       }
     };
 
-    it('should validate empty name', () => {
-      testValidator('', true);
-    });
+    it('should validate empty name', fakeAsync(() => {
+      formHelper.expectErrorChange('bid', '', 'required', true);
+    }));
 
-    it('bucket names cannot be formatted as IP address', () => {
+    it('bucket names cannot be formatted as IP address', fakeAsync(() => {
       testValidator('172.10.4.51', false);
-    });
+    }));
 
-    it('bucket name must be >= 3 characters long (1/2)', () => {
+    it('bucket name must be >= 3 characters long (1/2)', fakeAsync(() => {
       testValidator('ab', false);
-    });
+    }));
 
-    it('bucket name must be >= 3 characters long (2/2)', () => {
+    it('bucket name must be >= 3 characters long (2/2)', fakeAsync(() => {
       testValidator('abc', true);
-    });
+    }));
 
-    it('bucket name must be <= than 63 characters long (1/2)', () => {
+    it('bucket name must be <= than 63 characters long (1/2)', fakeAsync(() => {
       testValidator(_.repeat('a', 64), false);
-    });
+    }));
 
-    it('bucket name must be <= than 63 characters long (2/2)', () => {
+    it('bucket name must be <= than 63 characters long (2/2)', fakeAsync(() => {
       testValidator(_.repeat('a', 63), true);
-    });
+    }));
 
-    it('bucket names must not contain uppercase characters or underscores (1/2)', () => {
+    it('bucket names must not contain uppercase characters or underscores (1/2)', fakeAsync(() => {
       testValidator('iAmInvalid', false);
-    });
+    }));
 
-    it('bucket names must not contain uppercase characters or underscores (2/2)', () => {
+    it('bucket names must not contain uppercase characters or underscores (2/2)', fakeAsync(() => {
       testValidator('i_am_invalid', false);
-    });
+    }));
 
-    it('bucket names with invalid labels (1/3)', () => {
+    it('bucket names with invalid labels (1/3)', fakeAsync(() => {
       testValidator('abc.1def.Ghi2', false);
-    });
+    }));
 
-    it('bucket names with invalid labels (2/3)', () => {
-      testValidator('abc.1-xy', false);
-    });
+    it('bucket names with invalid labels (2/3)', fakeAsync(() => {
+      testValidator('abc.1_xy', false);
+    }));
 
-    it('bucket names with invalid labels (3/3)', () => {
+    it('bucket names with invalid labels (3/3)', fakeAsync(() => {
       testValidator('abc.*def', false);
-    });
+    }));
 
-    it('bucket names must be a series of one or more labels and can contain lowercase letters, numbers, and hyphens (1/3)', () => {
+    it('bucket names must be a series of one or more labels and can contain lowercase letters, numbers, and hyphens (1/3)', fakeAsync(() => {
       testValidator('xyz.abc', true);
-    });
+    }));
 
-    it('bucket names must be a series of one or more labels and can contain lowercase letters, numbers, and hyphens (2/3)', () => {
+    it('bucket names must be a series of one or more labels and can contain lowercase letters, numbers, and hyphens (2/3)', fakeAsync(() => {
       testValidator('abc.1-def', true);
-    });
+    }));
 
-    it('bucket names must be a series of one or more labels and can contain lowercase letters, numbers, and hyphens (3/3)', () => {
+    it('bucket names must be a series of one or more labels and can contain lowercase letters, numbers, and hyphens (3/3)', fakeAsync(() => {
       testValidator('abc.ghi2', true);
-    });
+    }));
 
-    it('bucket names must be unique', () => {
-      spyOn(rgwBucketService, 'enumerate').and.returnValue(observableOf(['abcd']));
-      const validatorFn = component.bucketNameValidator();
-      const ctrl = new FormControl('abcd');
-      ctrl.markAsDirty();
-      const validatorPromise = validatorFn(ctrl);
-      expect(validatorPromise instanceof Promise).toBeTruthy();
-      if (validatorPromise instanceof Promise) {
-        validatorPromise.then((resp) => {
-          expect(resp instanceof Object).toBeTruthy();
-          expect(resp.bucketNameExists).toBeTruthy();
-        });
-      }
-    });
+    it('bucket names must be unique', fakeAsync(() => {
+      testValidator('bucket-name-is-unique', true);
+    }));
 
     it('should get zonegroup and placement targets', () => {
       const payload: Record<string, any> = {
index 1c51d701d12d70058356b66abcf4cd6f6f1c6bb0..bde9390dcf310c4f330b67b7a53dd6df3e34d050 100644 (file)
@@ -3,7 +3,8 @@ import { AbstractControl, AsyncValidatorFn, ValidationErrors, Validators } from
 import { ActivatedRoute, Router } from '@angular/router';
 
 import _ from 'lodash';
-import { forkJoin } from 'rxjs';
+import { forkJoin, Observable, of as observableOf, timer as observableTimer } from 'rxjs';
+import { map, switchMapTo } from 'rxjs/operators';
 
 import { RgwBucketService } from '~/app/shared/api/rgw-bucket.service';
 import { RgwSiteService } from '~/app/shared/api/rgw-site.service';
@@ -242,59 +243,67 @@ export class RgwBucketFormComponent extends CdForm implements OnInit {
    *   start and end with a lowercase letter or a number.
    */
   bucketNameValidator(): AsyncValidatorFn {
-    const rgwBucketService = this.rgwBucketService;
-    return (control: AbstractControl): Promise<ValidationErrors | null> => {
-      return new Promise((resolve) => {
-        // Exit immediately if user has not interacted with the control yet
-        // or the control value is empty.
-        if (control.pristine || control.value === '') {
-          resolve(null);
-          return;
+    return (control: AbstractControl): Observable<ValidationErrors | null> => {
+      // Exit immediately if user has not interacted with the control yet
+      // or the control value is empty.
+      if (control.pristine || control.value === '') {
+        return observableOf(null);
+      }
+      const constraints = [];
+      // - Bucket names cannot be formatted as IP address.
+      constraints.push(() => {
+        const ipv4Rgx = /^((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$/i;
+        const ipv6Rgx = /^(?:[a-f0-9]{1,4}:){7}[a-f0-9]{1,4}$/i;
+        const name = this.bucketForm.get('bid').value;
+        let notAnIP = true;
+        if (ipv4Rgx.test(name) || ipv6Rgx.test(name)) {
+          notAnIP = false;
         }
-        const constraints = [];
-        // - Bucket names cannot be formatted as IP address.
-        constraints.push((name: AbstractControl) => {
-          const validatorFn = CdValidators.ip();
-          return !validatorFn(name);
-        });
-        // - Bucket names can be between 3 and 63 characters long.
-        constraints.push((name: string) => _.inRange(name.length, 3, 64));
-        // - Bucket names must not contain uppercase characters or underscores.
-        // - Bucket names must start with a lowercase letter or number.
-        // - Bucket names must be a series of one or more labels. Adjacent
-        //   labels are separated by a single period (.). Bucket names can
-        //   contain lowercase letters, numbers, and hyphens. Each label must
-        //   start and end with a lowercase letter or a number.
-        constraints.push((name: string) => {
-          const labels = _.split(name, '.');
-          return _.every(labels, (label) => {
-            // Bucket names must not contain uppercase characters or underscores.
-            if (label !== _.toLower(label) || label.includes('_')) {
-              return false;
-            }
-            // Bucket names can contain lowercase letters, numbers, and hyphens.
-            if (!/[0-9a-z-]/.test(label)) {
-              return false;
-            }
-            // Each label must start and end with a lowercase letter or a number.
-            return _.every([0, label.length], (index) => {
-              return /[a-z]/.test(label[index]) || _.isInteger(_.parseInt(label[index]));
-            });
+        return notAnIP;
+      });
+      // - Bucket names can be between 3 and 63 characters long.
+      constraints.push((name: string) => _.inRange(name.length, 3, 64));
+      // - Bucket names must not contain uppercase characters or underscores.
+      // - Bucket names must start with a lowercase letter or number.
+      // - Bucket names must be a series of one or more labels. Adjacent
+      //   labels are separated by a single period (.). Bucket names can
+      //   contain lowercase letters, numbers, and hyphens. Each label must
+      //   start and end with a lowercase letter or a number.
+      constraints.push((name: string) => {
+        const labels = _.split(name, '.');
+        return _.every(labels, (label) => {
+          // Bucket names must not contain uppercase characters or underscores.
+          if (label !== _.toLower(label) || label.includes('_')) {
+            return false;
+          }
+          // Bucket names can contain lowercase letters, numbers, and hyphens.
+          if (!/[0-9a-z-]/.test(label)) {
+            return false;
+          }
+          // Each label must start and end with a lowercase letter or a number.
+          return _.every([0, label.length], (index) => {
+            return /[a-z]/.test(label[index]) || _.isInteger(_.parseInt(label[index]));
           });
         });
-        if (!_.every(constraints, (func: Function) => func(control.value))) {
-          resolve({ bucketNameInvalid: true });
-          return;
-        }
-        // - Bucket names must be unique.
-        rgwBucketService.exists(control.value).subscribe((resp: boolean) => {
+      });
+      if (!_.every(constraints, (func: Function) => func(control.value))) {
+        return observableTimer().pipe(
+          map(() => {
+            return { bucketNameInvalid: true };
+          })
+        );
+      }
+      // - Bucket names must be unique.
+      return observableTimer().pipe(
+        switchMapTo(this.rgwBucketService.exists.call(this.rgwBucketService, control.value)),
+        map((resp: boolean) => {
           if (!resp) {
-            resolve(null);
+            return null;
           } else {
-            resolve({ bucketNameExists: true });
+            return { bucketNameExists: true };
           }
-        });
-      });
+        })
+      );
     };
   }
 
index 6bb86dd4cda8f69ff87794384ff43f05c0d5ddbb..15665d53bb95b704b2c62a9eef65e6537a836d3c 100644 (file)
@@ -162,14 +162,14 @@ describe('RgwUserFormComponent', () => {
     it('should validate that username is valid', fakeAsync(() => {
       spyOn(rgwUserService, 'get').and.returnValue(throwError('foo'));
       formHelper.setValue('user_id', 'ab', true);
-      tick(500);
+      tick();
       formHelper.expectValid('user_id');
     }));
 
     it('should validate that username is invalid', fakeAsync(() => {
       spyOn(rgwUserService, 'get').and.returnValue(observableOf({}));
       formHelper.setValue('user_id', 'abc', true);
-      tick(500);
+      tick();
       formHelper.expectError('user_id', 'notUnique');
     }));
   });
index 3f2bae5f48a1658608176ad44f7e54775974aaba..32c88b8c947511724732b7203461c49fa5ba94d7 100644 (file)
@@ -80,7 +80,7 @@ describe('RgwBucketService', () => {
     service.exists('foo').subscribe((resp) => {
       result = resp;
     });
-    const req = httpTesting.expectOne(`api/rgw/bucket?${RgwHelper.DAEMON_QUERY_PARAM}`);
+    const req = httpTesting.expectOne(`api/rgw/bucket/foo?${RgwHelper.DAEMON_QUERY_PARAM}`);
     expect(req.request.method).toBe('GET');
     req.flush(['foo', 'bar']);
     expect(result).toBe(true);
index 1d622518bba905978e9e62566a984f9acb1a0e8e..03fe5ca017dcec39ac6f046587d4999645cf604d 100644 (file)
@@ -3,7 +3,7 @@ import { Injectable } from '@angular/core';
 
 import _ from 'lodash';
 import { of as observableOf } from 'rxjs';
-import { mergeMap } from 'rxjs/operators';
+import { catchError, mapTo } from 'rxjs/operators';
 
 import { RgwDaemonService } from '~/app/shared/api/rgw-daemon.service';
 import { cdEncode } from '~/app/shared/decorators/cd-encode';
@@ -28,16 +28,6 @@ export class RgwBucketService {
     });
   }
 
-  /**
-   * Get the list of bucket names.
-   * @return Observable<string[]>
-   */
-  enumerate() {
-    return this.rgwDaemonService.request((params: HttpParams) => {
-      return this.http.get(this.url, { params: params });
-    });
-  }
-
   get(bucket: string) {
     return this.rgwDaemonService.request((params: HttpParams) => {
       return this.http.get(`${this.url}/${bucket}`, { params: params });
@@ -112,10 +102,13 @@ export class RgwBucketService {
    * @return Observable<boolean>
    */
   exists(bucket: string) {
-    return this.enumerate().pipe(
-      mergeMap((resp: string[]) => {
-        const index = _.indexOf(resp, bucket);
-        return observableOf(-1 !== index);
+    return this.get(bucket).pipe(
+      mapTo(true),
+      catchError((error: Event) => {
+        if (_.isFunction(error.preventDefault)) {
+          error.preventDefault();
+        }
+        return observableOf(false);
       })
     );
   }
index 1537c1500045882ec8e588bf771cfe92369f2c1e..b72b8f18bdcf5c62054bd3680cce37099093d550 100644 (file)
@@ -348,29 +348,26 @@ export class CdValidators {
     serviceFn: existsServiceFn,
     serviceFnThis: any = null,
     usernameFn?: Function,
-    uidfield = false,
-    dueTime = 500
+    uidField = false
   ): AsyncValidatorFn {
-    let uname: string;
+    let uName: string;
     return (control: AbstractControl): Observable<ValidationErrors | null> => {
       // Exit immediately if user has not interacted with the control yet
       // or the control value is empty.
       if (control.pristine || isEmptyInputValue(control.value)) {
         return observableOf(null);
       }
-      uname = control.value;
+      uName = control.value;
       if (_.isFunction(usernameFn) && usernameFn() !== null && usernameFn() !== '') {
-        if (uidfield) {
-          uname = `${control.value}$${usernameFn()}`;
+        if (uidField) {
+          uName = `${control.value}$${usernameFn()}`;
         } else {
-          uname = `${usernameFn()}$${control.value}`;
+          uName = `${usernameFn()}$${control.value}`;
         }
       }
 
-      // Forgot previous requests if a new one arrives within the specified
-      // delay time.
-      return observableTimer(dueTime).pipe(
-        switchMapTo(serviceFn.call(serviceFnThis, uname)),
+      return observableTimer().pipe(
+        switchMapTo(serviceFn.call(serviceFnThis, uName)),
         map((resp: boolean) => {
           if (!resp) {
             return null;