[I] createSmartNumberFormatter drops a caller-supplied id and mislabels the formatter as signed [superset]

["krishn1301 (via GitHub)" <gi...@apache.org>]

krishn1301 opened a new issue, #43430:
URL: https://github.com/apache/superset/issues/43430

   ### Bug description
   
   `createSmartNumberFormatter()` discards a caller-supplied `id` and returns a formatter whose `id` is `SMART_NUMBER_SIGNED`, even when `signed` was never set. The resulting object is internally inconsistent: it identifies as the *signed* formatter but does not sign anything.
   
   The `id` and `label` options are part of the exported public API of `@superset-ui/core`, so any plugin registering its own adaptive formatter hits this.
   
   ### How to reproduce
   
   ```ts
   import { createSmartNumberFormatter } from '@superset-ui/core';
   
   const f = createSmartNumberFormatter({ id: 'my_custom_format', label: 'My format' });
   
   f.id     // 'SMART_NUMBER_SIGNED'   <- expected 'my_custom_format'
   f(5)     // '5'                     <- no '+', so it is not actually a signed formatter
   f.label  // 'My format'             <- label is handled correctly
   ```
   
   Run against `9f505eb0`:
   
   ```
   passed id 'my_custom_format', signed omitted -> formatter.id = 'SMART_NUMBER_SIGNED'
   formats 5 as '5' (no '+', so it is NOT a signed formatter)
   ```
   
   ### Expected results
   
   `f.id === 'my_custom_format'`, and the `signed` flag alone decides between `SMART_NUMBER` and `SMART_NUMBER_SIGNED` when no `id` is supplied.
   
   ### Actual results
   
   The custom `id` is dropped and replaced with `SMART_NUMBER_SIGNED`.
   
   ### Root cause
   
   `superset-frontend/packages/superset-ui-core/src/number-format/factories/createSmartNumberFormatter.ts`:
   
   ```ts
       id:
         id || signed
           ? NumberFormats.SMART_NUMBER_SIGNED
           : NumberFormats.SMART_NUMBER,
       label: label ?? 'Adaptive formatter',
   ```
   
   `||` binds tighter than `?:`, so this parses as:
   
   ```ts
   id: (id || signed) ? SMART_NUMBER_SIGNED : SMART_NUMBER
   ```
   
   Any truthy `id` therefore selects the *signed* branch instead of being used as the id. The intended grouping is almost certainly:
   
   ```ts
   id: id ?? (signed ? NumberFormats.SMART_NUMBER_SIGNED : NumberFormats.SMART_NUMBER),
   ```
   
   The `label` line immediately below already uses `??` for exactly this "caller value wins, otherwise fall back" shape, which suggests the `id` line is a precedence slip rather than intent.
   
   Truth table for the current expression:
   
   | `id` | `signed` | current `.id` | expected |
   | --- | --- | --- | --- |
   | omitted | `false` | `SMART_NUMBER` | `SMART_NUMBER` |
   | omitted | `true` | `SMART_NUMBER_SIGNED` | `SMART_NUMBER_SIGNED` |
   | `'my_fmt'` | `false` | `SMART_NUMBER_SIGNED` | `'my_fmt'` |
   | `'my_fmt'` | `true` | `SMART_NUMBER_SIGNED` | `'my_fmt'` |
   
   Only the two default rows are correct, and those are the only two the current tests and internal callers exercise.
   
   ### Scope
   
   Formatting output is **not** affected — `formatFunc` reads `signed` directly, so numbers render correctly in every case. The damage is confined to the formatter's identity:
   
   - A plugin registering the formatter under its own id gets an object whose `.id` disagrees with its registry key.
   - Anything keying off `.id` to decide whether a formatter is signed will be wrong.
   
   Both in-tree call sites (`NumberFormatterRegistry`) pass no `id`, so **nothing in Superset itself misbehaves today**. This is a latent defect in exported API surface.
   
   The existing suite (`test/number-format/factories/createSmartNumberFormatter.test.ts`) never passes `id` or `label`, which is why it stays green.
   
   ### Environment
   
   - master at `9f505eb0`
   - Reproduced with the repo's own Jest setup, no patches
   
   ### Checklist
   
   - [x] I have searched Superset's GitHub issues and pull requests and found no report of this
   - [x] I have reproduced the issue on current `master`
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [I] createSmartNumberFormatter drops a caller-supplied id and mislabels the formatter as signed [superset]

["sadpandajoe (via GitHub)" <gi...@apache.org>]

sadpandajoe closed issue #43430: createSmartNumberFormatter drops a caller-supplied id and mislabels the formatter as signed
URL: https://github.com/apache/superset/issues/43430


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org