[Painless Lab] Fix float -> integer coercion bug #60201

cjcenizal · 2020-03-14T16:57:16Z

Summary

The original problem was caused by the use of objects for storing state, e.g. the parameters or document configuration. This resulted in a parameters definition that originally looked like this:

{
  "count": 100.0,
  "total": 1000.0
}

To become this:

{
  "count": 100,
  "total": 1000
}

ES would then treat these parameters as integers, which prevented operations like count / total from having any level of floating point precision.

The solution is to store input as a string instead of an object, and to preserve these values as strings on the journey from client to API to ES.

Other notes

Drawbacks

One downside of this change is that the rendered API request has wonky indentation. We'll need to figure out some way to inject indentation into the interpolated parts of the request body.

To-do

Set code editors, e.g. parameters editor, to indent JSON by two spaces

elasticmachine · 2020-03-14T16:57:18Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

- Send strings to API and ES client instead of objects.

jloleysens

Great job @cjcenizal ! Happy with these changes. This is an issue any JS client will face s = 1.0 is coerced to 1, which is why I guess they allow submitting a string as a body to the execute endpoint.

It is strange to me that the "count" and "total" lines are correctly aligned, but not the }. Where does the extra white space padding on the left come from?

jloleysens · 2020-03-16T08:07:45Z

x-pack/plugins/painless_lab/server/routes/api/execute.ts

+// const bodySchema = schema.object({
+//   script: schema.object({
+//     source: schema.string(),
+//     params: schema.maybe(schema.recordOf(schema.string(), schema.any())),
+//   }),
+//   context: schema.maybe(schema.string()),
+//   context_setup: schema.maybe(
+//     schema.object({
+//       index: schema.string(),
+//       document: schema.string(),
+//     })
+//   ),
+// });


Suggested change

// const bodySchema = schema.object({

// script: schema.object({

// source: schema.string(),

// params: schema.maybe(schema.recordOf(schema.string(), schema.any())),

// }),

// context: schema.maybe(schema.string()),

// context_setup: schema.maybe(

// schema.object({

// index: schema.string(),

// document: schema.string(),

// })

// ),

// });

jloleysens · 2020-03-16T08:08:13Z

x-pack/plugins/painless_lab/server/routes/api/execute.ts

@@ -37,7 +37,7 @@ export function registerExecuteRoute({ router, license }: RouteDependencies) {

      try {
        const callAsCurrentUser = ctx.core.elasticsearch.dataClient.callAsCurrentUser;
-
+        console.log('body', body);


This should also be removed

jloleysens · 2020-03-16T08:15:13Z

x-pack/plugins/painless_lab/public/application/hooks/use_submit_code.ts

+          const result = await http.post(`${API_BASE_PATH}/execute`, {
+            // Stringify the string, because http runs it through JSON.parse, and we want to actually
+            // send a JSON string.
+            body: JSON.stringify(buildRequestPayload(config)),


I think it could be even more clear if we do the double stringification here with the comment. JSON.stringify(JSON.stringify(...)

Ah, nvm I see we are building this up manually in buildRequestPayload.

jloleysens · 2020-03-16T08:34:08Z

x-pack/plugins/painless_lab/public/application/components/output_pane/context_tab.tsx

+  context: any;
+  index: string;
+  document: string;
+  onContextChange: (change: any) => void;


nit: can we add types here for change?

cjcenizal · 2020-03-17T02:03:13Z

Thanks for catching those mistakes @jloleysens! I fixed the indentation problem too.

kibanamachine · 2020-03-17T04:04:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 1f140c4

History

💔 Build #33271 failed 344df24
💔 Build #33264 failed 006f602127d42517406e8492cebbb05e32b9740e

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* Create Painless Playground app (#54578) * Replace heart script with smiley face script. (#57755) * Rename Painless Playground -> Painless Lab. (#57545) * Fix i18n namespace. * Improve smiley face proportions. - Add def keyword to Painless spec. - Temporarily fix broken highlighting. - Add small padding to main controls. * [Painless Lab] Minor Fixes (#58135) * Code restructure, improve types, add plugin id, introduced hook Moved the code execution hook to a custom hook outside of main, also chaining off promise to avoid lower level handling of sequencing. * Re-instated formatting code To improve DX the execution error response from the painless API was massaged to a more reader friendly state, only giving non-repeating information. Currently it is hard to determine the line and character information from the painless endpoint. If the user wishes to see this raw information it will be available in the API response flyout. * Remove leading new line in default script * Remove registration of feature flag * Fix types * Restore previous auto-submit request behaviour * Remove use of null and remove old comment Stick with "undefined" as the designation for something not existing. * [Painless Lab] NP migration (#59794) * Fix sample document editor. * [Painless Lab] Fix float -> integer coercion bug (#60201) * Clarify data and persistence flow. Fix floating point precision bug. * Send a string to API and ES client instead of an object. * Rename helpers lib to format. Add tests for formatRequestPayload. * Add query parameter to score context (#60414) * Fix typo and i18n * Make state init lazy Otherwise we are needlessly reading and JSON.parse'ing on every state update * Support the query parameter in requests to Painless * Fix borked i18n * Fix i18n * Another i18n issue * [Painless] Minor state update model refactor (#60532) * Fix typo and i18n * Make state init lazy Otherwise we are needlessly reading and JSON.parse'ing on every state update * Support the query parameter in requests to Painless * WiP on state refactor * Some cleanup after manual testing * Fix types and i18n * Fix i18n in context_tab * i18n * [Painless] Language Service (#60612) * Added language service * Use the correct monaco instance and add wordwise operations * Remove plugin context initializer for now * [Painless] Replace hard-coded links (#60603) * Replace hard-coded links Also remove all props from Main component * Pass the new links object to the request flyout too * Link directly to painless execute API's contexts * Remove responsive stacking from tabs with icons in them. * Resize Painless Lab bottom bar to accommodate nav drawer width (#60833) * Validate Painless Lab index field (#60841) * Make JSON format of parameters field more prominent. Set default parameters to provide an example to users. * Set default document to provide an example to users. * Simplify context's updateState interface. * Refactor store and context file organization. - Remove common directory, move constants and types files to root. - Move initialState into context file, where it's being used. * Add validation for index input. * Create context directory. * Fix bottom bar z-index. * Position flyout help link so it's bottom-aligned with the title and farther from the close button. Co-authored-by: Matthias Wilhelm <[email protected]> Co-authored-by: Jean-Louis Leysens <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Alison Goryachev <[email protected]>

* Create Painless Playground app (#54578) * Replace heart script with smiley face script. (#57755) * Rename Painless Playground -> Painless Lab. (#57545) * Fix i18n namespace. * Improve smiley face proportions. - Add def keyword to Painless spec. - Temporarily fix broken highlighting. - Add small padding to main controls. * [Painless Lab] Minor Fixes (#58135) * Code restructure, improve types, add plugin id, introduced hook Moved the code execution hook to a custom hook outside of main, also chaining off promise to avoid lower level handling of sequencing. * Re-instated formatting code To improve DX the execution error response from the painless API was massaged to a more reader friendly state, only giving non-repeating information. Currently it is hard to determine the line and character information from the painless endpoint. If the user wishes to see this raw information it will be available in the API response flyout. * Remove leading new line in default script * Remove registration of feature flag * Fix types * Restore previous auto-submit request behaviour * Remove use of null and remove old comment Stick with "undefined" as the designation for something not existing. * [Painless Lab] NP migration (#59794) * Fix sample document editor. * [Painless Lab] Fix float -> integer coercion bug (#60201) * Clarify data and persistence flow. Fix floating point precision bug. * Send a string to API and ES client instead of an object. * Rename helpers lib to format. Add tests for formatRequestPayload. * Add query parameter to score context (#60414) * Fix typo and i18n * Make state init lazy Otherwise we are needlessly reading and JSON.parse'ing on every state update * Support the query parameter in requests to Painless * Fix borked i18n * Fix i18n * Another i18n issue * [Painless] Minor state update model refactor (#60532) * Fix typo and i18n * Make state init lazy Otherwise we are needlessly reading and JSON.parse'ing on every state update * Support the query parameter in requests to Painless * WiP on state refactor * Some cleanup after manual testing * Fix types and i18n * Fix i18n in context_tab * i18n * [Painless] Language Service (#60612) * Added language service * Use the correct monaco instance and add wordwise operations * Remove plugin context initializer for now * [Painless] Replace hard-coded links (#60603) * Replace hard-coded links Also remove all props from Main component * Pass the new links object to the request flyout too * Link directly to painless execute API's contexts * Remove responsive stacking from tabs with icons in them. * Resize Painless Lab bottom bar to accommodate nav drawer width (#60833) * Validate Painless Lab index field (#60841) * Make JSON format of parameters field more prominent. Set default parameters to provide an example to users. * Set default document to provide an example to users. * Simplify context's updateState interface. * Refactor store and context file organization. - Remove common directory, move constants and types files to root. - Move initialState into context file, where it's being used. * Add validation for index input. * Create context directory. * Fix bottom bar z-index. * Position flyout help link so it's bottom-aligned with the title and farther from the close button. Co-authored-by: Matthias Wilhelm <[email protected]> Co-authored-by: Jean-Louis Leysens <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Alison Goryachev <[email protected]> Co-authored-by: Matthias Wilhelm <[email protected]> Co-authored-by: Jean-Louis Leysens <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Alison Goryachev <[email protected]>

cjcenizal added Feature:Dev Tools painless painless Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes labels Mar 14, 2020

cjcenizal force-pushed the bug/painless-data-flow branch from 006f602 to 7b931ea Compare March 14, 2020 18:45

cjcenizal changed the title ~~[Painless Lab] Clarify data and persistence flow.~~ [Painless Lab] Fix float -> integer coercion bug Mar 14, 2020

Clarify data and persistence flow. Fix floating point precision bug.

344df24

- Send strings to API and ES client instead of objects.

cjcenizal force-pushed the bug/painless-data-flow branch from 7b931ea to 344df24 Compare March 14, 2020 19:28

cjcenizal marked this pull request as ready for review March 15, 2020 20:24

cjcenizal requested a review from jloleysens March 15, 2020 20:24

jloleysens approved these changes Mar 16, 2020

View reviewed changes

Address feedback and fix bugs with payload formatting helper.

1f140c4

cjcenizal merged commit 3de6244 into elastic:app/painless Mar 17, 2020

cjcenizal deleted the bug/painless-data-flow branch March 17, 2020 02:03

cjcenizal mentioned this pull request Mar 23, 2020

[Painless Lab] Floating point values coerced to integers #60140

Closed

cjcenizal added the Feature:Painless Lab Dev tool for learning Painless label Mar 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Painless Lab] Fix float -> integer coercion bug #60201

[Painless Lab] Fix float -> integer coercion bug #60201

cjcenizal commented Mar 14, 2020 •

edited

Loading

elasticmachine commented Mar 14, 2020

jloleysens left a comment

jloleysens Mar 16, 2020

jloleysens Mar 16, 2020

jloleysens Mar 16, 2020

jloleysens Mar 16, 2020

jloleysens Mar 16, 2020

cjcenizal commented Mar 17, 2020

kibanamachine commented Mar 17, 2020

[Painless Lab] Fix float -> integer coercion bug #60201

[Painless Lab] Fix float -> integer coercion bug #60201

Conversation

cjcenizal commented Mar 14, 2020 • edited Loading

Summary

Other notes

Drawbacks

To-do

elasticmachine commented Mar 14, 2020

jloleysens left a comment

Choose a reason for hiding this comment

jloleysens Mar 16, 2020

Choose a reason for hiding this comment

jloleysens Mar 16, 2020

Choose a reason for hiding this comment

jloleysens Mar 16, 2020

Choose a reason for hiding this comment

jloleysens Mar 16, 2020

Choose a reason for hiding this comment

jloleysens Mar 16, 2020

Choose a reason for hiding this comment

cjcenizal commented Mar 17, 2020

kibanamachine commented Mar 17, 2020

💚 Build Succeeded

History

cjcenizal commented Mar 14, 2020 •

edited

Loading