Advanced Settings - Image Input by kobelb · Pull Request #15342 · elastic/kibana

kobelb · 2017-12-01T16:05:33Z

Adds an "image" advanced setting type. The maximum size is currently hard-coded for all images to 200kB, we could potentially make this configurable and allow the uiSetting configuration to specify this value, but I figured 200kB was reasonable for the time being. The inputBaseSixtyFour only supports a single file for the time being, and will throw an Error that results in a "fatal error" if it's given multiple files.

I used RxJs/Observables for the inputBaseSixtyFour directive so that we could abort the FileReader when the change event is fired before the reading has finished.

"Release Note: Added image support to the Advanced Settings"

kobelb · 2017-12-01T16:06:27Z

@spalger I tagged you as a reviewer, since you're the Observables expert, but if you'd like me to defer to someone else, just lemme know.

jbudz · 2017-12-01T16:37:15Z

other side note, feel free to ignore: you could defer file size to the server's max payload size config. I'll stop intruding on this - sorry!

kobelb · 2017-12-01T16:41:52Z

@jbudz please don't stop intruding! I figured we'd want to stop huge images from getting into the .kibana "config" document, and might want more fine grained control of this, and a more pleasant user experience, do you think 200kb is too small?

jbudz · 2017-12-01T17:00:47Z

Nope makes sense, I'd expect 200kb to be fine for a logo and if there's other uses we can always bump it up then.

spalger · 2017-12-01T20:23:10Z

you could defer file size to the server's max payload size config.

I think it's important that we enforce this validation on the server too, if we really want there to be a limit

spalger · 2017-12-01T20:25:27Z

src/ui/public/directives/input_base64.js

+import { Observable } from 'rxjs/Rx';
+const module = uiModules.get('kibana');
+
+const $readFileContents = (file) => {


Personal option, createFileContent$ (create file content stream) reads a little better, especially when you're assigning it to a variable:

const fileContent$ = createFileContent$(file)

Makes sense, I will make it so.

spalger · 2017-12-01T20:28:44Z

src/ui/public/directives/input_base64.js

+    const reader = new FileReader();
+    reader.onerror = (err) => {
+      observer.error(err);
+      observer.complete();


observer.error() is like rejecting a promise, calling observer.complete() afterward does nothing.

Good point, I'll remove the unnecessary completion.

spalger

LGTM, tested by adding a logo to Kibanas uiSettings https://gist.github.com/anonymous/b2c3a92fee83fc7749af8113349258e7

spalger · 2017-12-01T20:41:31Z

src/ui/public/directives/input_base64.js

+        .map(dataUrl => {
+          const validations = validators.map(validator => validator(dataUrl));
+
+          return { dataUrl, validations };


More opinions: when I need to carry data onto another part of a stream I break up the original stream rather than require the previous stream to pass on it's input value. It might look something like this here:

// produce fileContent$ whenever the $element 'change' event is triggered. const fileContent$ = Observable .fromEvent($elem, 'change') .map(e => e.target.files) .switchMap(files => { if (files.length === 0) { return []; } if (files.length > 1) { throw new Error('Only one file is supported at this time'); } return $readFileContents(files[0]); }) .share(); // validate the content of the files after it is loaded const validations$ = fileContent$ .map(fileContent => ( validators.map(validator => validator(fileContent)) )) .toArray() // push results from input/validation to the ngModel const unsubscribe = Observable .combineLatest(fileContent$, validations$) .subscribe(([ fileContent, validations ]) => { $scope.$evalAsync(() => { ... }) })

I like it, more readable! I will make it so.

spalger

Sorry, mixed signals, I think we should enforce the size limit on the server too, perhaps the max image size should come from the config value, maybe something like this: https://gist.github.com/anonymous/c5b669c4df1ab0a813670a217b3ea83a

kobelb · 2017-12-01T21:10:53Z

@spalger wrt to the server enforcing the size limit, that makes sense, I'll make those changes.

…view

spalger · 2017-12-01T21:34:42Z

src/ui/public/directives/input_base64.js

+          }
+
+          if (files.length > 1) {
+            throw new Error('Only one file is supported at this time');


Couldn't this be a validator rather than a fatal error?

Also, is it possible to submit multiple files since the input[type=file] requires the multiple attribute to enable multi-selection?

I don't really want this error to be displayed to the end-user as much as the developer who tries to use it on an input with the multiple attribute, so I don't think it makes much sense to have it as a validator.

We could, potentially, but it makes the code more complicated to support multiple files (when we don't really need it yet) so I'm throwing "better" errors more places to make this more obvious.

won't really do anything for other data-types

kobelb · 2017-12-04T17:20:27Z

@spalger I'm now enforcing the maximum size of the setting on the server as well, using a configurable value specified like:

maxSize: {
  length: kbToBase64Length(200),
  description: '200 kB'
}

The UiSettingsService itself is enforcing this and throwing an InvalidValueError when this is violated. Given the way that the uiSettings themselves are implemented, I didn't see a great way to share validation rules between the server/client, because the "uiSettings" are serialized and included as part of the kbn-initial-state.

The "proper" solution to this would likely require a rather significant effort, does the current implementation seem reasonable enough for the time-being, or would you rather I investigate a more unified and robust manner for validating ui settings?

spalger · 2017-12-04T22:32:55Z

src/ui/ui_settings/routes/set.js

+    await uiSettings.set(key, value);
+  } catch (err) {
+    if (err instanceof InvalidValueError) {
+      return boom.badRequest(err.message);


throw should be appropriate here

spalger · 2017-12-04T22:36:35Z

src/ui/ui_settings/ui_settings_service.js

+ * Invalid value for UiSetting
+ * @class InvalidValudError
+ */
+export class InvalidValueError extends Error {


We have a fairly established convention for handling service-specific errors. See:

SavedObjectsClient/errors

SavedObjectsClient#errors

Is this an approach that we intend to support with the new platform? Throwing boom errors, which are http specific, seems like it won't scale that well as we start to support more "background" tasks that aren't attached to a specific http request/response.

spalger · 2017-12-04T22:39:20Z

src/ui/ui_settings/ui_settings_service.js

+      const validations = defaults[key].validations;
+      const value = changes[key];
+      for (const validation of Object.values(validations)) {
+        const result = validation.schema.validate(value);


I think it's important to capture result.value here, since JoiSchema#validate() will return defaults and such if they are configured. Also, why not have a single joi schema for the value? Joi can merge multiple validations into a single error message and if you use abortEarly: false it will list all errors rather than just the first.

spalger

Hmm, I'm a little concerned by the amount of flexibility implemented currently. I'm not opposed to having validation be a feature of the UiSettings service, but the fact that we are supporting the Joi JSON-DSL in the uiSettingDefaults means we're kind of adding a whole new DSL to our API. I wouldn't expect support for Joi to follow us into the new platform so I'm thinking we should narrow the scope of the solution here a bit... Thoughts?

kobelb · 2017-12-05T13:33:50Z

@spalger narrowing the solution seems fine to me, I has hesitant to add such flexible validation support to uiSettings, and at one point I was just hard-coding the maxLength validation here but it felt like this was bolted on, and it really only worked well for strings, since we're checking the "length" of the value.

There's also a commit before switching to joi where I was just returning { isValid: boolean, message: optional<string> }, do either of these seem like a good middle-ground?

spalger · 2017-12-06T01:04:50Z

I personally think that having type-specific validation options is fine, so maxSizeKb would only be supported for images, and UiSettingsService#setMany would just validate step through each value being set, see if it's an image, and then check it's maxSizeKb. Using a function means that we don't have static values to use in the UI, right? It won't scale if we decided to add a ton of validation to the uiSettings, but I don't think we need to worry about that now.

PS: I like maxSizeKb because it means that the UI can easily describe the value as kilobytes, and it limits the amount of customization intentionally.

PPS: Since a setting like maxSizeKb must assume that the data is base64 encoded in order to properly check the size in KB we might as well ensure that the value is base64 encoded too.

spalger

LGTM

* Beginning to work on image advanced setting type * Making the maxSize code and UI more user-friendly (hopefully). * Getting rid of that border, I was using the wrong class * Displaying an image for the preview * Not displaying the image if we don't have a value * Removing no longer used code * Adding explicit throw in the subscription to make our intents more clear * Changing some of the Observable style based on input from the peer review * Using the maxSizeKB option to enforce the size * Adding support for no max * Throwing better errors * No longer duplicated the multiple usage error message * Using the maxSize attribute to control the max size of the settings * No longer using JSON.stringify, it's already a string and that approach won't really do anything for other data-types * Extracting validate to it's own method * No longer hardcoding the maxLength validation in the uiSettings * Using a Joi schema instead * Punting on server-side validation for the time being

kobelb added 8 commits November 29, 2017 07:34

Beginning to work on image advanced setting type

1f25a46

Making the maxSize code and UI more user-friendly (hopefully).

9928e7e

Getting rid of that border, I was using the wrong class

cd37852

Displaying an image for the preview

158a30e

Not displaying the image if we don't have a value

c80424e

Removing no longer used code

c613e38

Merge remote-tracking branch 'upstream/master' into reporting-logo

28d074d

Adding explicit throw in the subscription to make our intents more clear

e84ba75

kobelb added v6.2.0 v7.0.0 labels Dec 1, 2017

kobelb requested review from spalger and stacey-gammon December 1, 2017 16:05

kobelb added the release_note:enhancement label Dec 1, 2017

spalger reviewed Dec 1, 2017

View reviewed changes

spalger approved these changes Dec 1, 2017

View reviewed changes

spalger suggested changes Dec 1, 2017

View reviewed changes

Changing some of the Observable style based on input from the peer re…

8456d55

…view

spalger reviewed Dec 1, 2017

View reviewed changes

kobelb added 6 commits December 1, 2017 16:59

Using the maxSizeKB option to enforce the size

ef8c2a9

Adding support for no max

9406acc

Throwing better errors

b17c3ba

No longer duplicated the multiple usage error message

110743f

Using the maxSize attribute to control the max size of the settings

921999e

No longer using JSON.stringify, it's already a string and that approach

b4300a9

won't really do anything for other data-types

kobelb added 3 commits December 4, 2017 12:22

Extracting validate to it's own method

63ae746

No longer hardcoding the maxLength validation in the uiSettings

8a36c64

Using a Joi schema instead

e484145

spalger reviewed Dec 4, 2017

View reviewed changes

spalger suggested changes Dec 4, 2017

View reviewed changes

Punting on server-side validation for the time being

2ee1e46

spalger approved these changes Dec 9, 2017

View reviewed changes

kobelb added 2 commits December 11, 2017 08:46

Merge remote-tracking branch 'upstream/master' into reporting-logo

842f8c7

Merge remote-tracking branch 'upstream/master' into reporting-logo

d3bd272

kobelb merged commit 4b0a9fc into elastic:master Jan 3, 2018

kobelb deleted the reporting-logo branch January 3, 2018 16:11

kobelb mentioned this pull request Jan 3, 2018

[6.x] Advanced Settings - Image Input (#15342) #15829

Merged

Conversation

kobelb commented Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kobelb commented Dec 1, 2017

Uh oh!

jbudz commented Dec 1, 2017

Uh oh!

kobelb commented Dec 1, 2017

Uh oh!

jbudz commented Dec 1, 2017

Uh oh!

spalger commented Dec 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger left a comment

Choose a reason for hiding this comment

Uh oh!

kobelb commented Dec 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kobelb commented Dec 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger Dec 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kobelb Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger Dec 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger left a comment

Choose a reason for hiding this comment

Uh oh!

kobelb commented Dec 5, 2017

Uh oh!

spalger commented Dec 6, 2017

Uh oh!

spalger left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kobelb commented Dec 1, 2017 •

edited

Loading

kobelb commented Dec 4, 2017 •

edited

Loading

spalger Dec 4, 2017 •

edited

Loading

kobelb Dec 5, 2017 •

edited

Loading

spalger Dec 4, 2017 •

edited

Loading