[WIP] feat: JSON Deserializer/Denormalizer #506

lal12 · 2023-02-23T13:27:01Z

Implements (half of) #86

Mostly done (I hope :D), but still have some issues with tests. Happy accepting advise.

Signed-off-by: Luca Lindhorst <[email protected]>

src/serialize/jsonDeserializer.ts

jkowalleck · 2023-02-23T15:21:44Z

src/serialize/json/denormalize.ts

+    if (spec === undefined) {
+      throw new UnsupportedFormatError(`Spec version ${data.specVersion} is not supported.`)
+    }
+    this.factory.spec = spec


https://github.com/CycloneDX/cyclonedx-javascript-library/pull/506/files#diff-facac0ec4128527fecf05b9814c75d81c08dfe23a5bc7c9f26611f6e224b3c51R112-R117

this should be part of the tool that does initial analysis.
reading which spec version is applied should be done before the factory is initialized.

you might wonder how the factory should be injectable, while it is not fully initialized?
possible solution: Inject a factory method, or the class type, instead of an object o the class.

src/serialize/json/denormalize.ts

jkowalleck · 2023-02-23T15:27:59Z

Thanks for your work,
I know there is still a lot to do, take your time.

Signed-off-by: Luca Lindhorst <[email protected]>

lal12 · 2023-03-02T14:15:22Z

Worked on the denormalizer. Probably less flexible than wished, but silently ignoring known properties if their type isn't right, does not seem like good behavior to me either.

No tests yet, but want feedback (@jkowalleck) on the general approach.

jkowalleck

reviewed as requested

remarks:

👍 I like the implementation of type assertions.
PS: they need improvement - see [WIP] feat: JSON Deserializer/Denormalizer #506 (comment)
❌ there is an option in the code, to change the #spec of a denormalizer Factory at runtime while this factory is in use. This side effect must be removed to prevent inconsistency.
👎 : I don't like the unforgiving behavior when data is not as expected. You could have an injectable optional warningFunc: (msg: string) => void that is called each time a value is unexpected or wrong, right before the value is ignored. It is important for the final result, to be able to generate a BOM model from partially invalid data. 💯 This feature could still be added later on.
note possible implementation - example code:
```
class Factory {
  readonly #spec: Spec
  readonly #warnFn: (msg: string) => void
  constructor(spec: Spec, warnFn?: (msg: string) => void) {
   this.#spec = spec
   this.#warnFn = warnFn ?? function () {/* noop */}
  }
 get warn () { return this.#warnFn } 
} 
myFactory = new Factory(...)
myFactory.warn('foooo baaarrr')
```

src/serialize/json/denormalize.ts

jkowalleck · 2023-03-02T20:35:06Z

src/serialize/json/denormalize.ts

+    if (spec === undefined) {
+      throw new UnsupportedFormatError(`Spec version ${data.specVersion} is not supported.`)
+    }
+    this.factory.spec = spec


you might wonder how the factory should be injectable, while it is not fully initialized?
possible solution: Inject a factory method, or the class type, instead of an object o the class.

jkowalleck · 2023-03-03T08:08:43Z

re: #506 (review)

@lal12 i have it more thought and i thing the type checkers you were implementing are technically correct, but do not help TypeScript with the type assertions.
Could you implement them as proper type assertions or type predicates?

read https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates
feature intro: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#assertion-functions

example:

function isString (v: any): v is string 
{
  return typeof v === 'string'
}

function assertIsString (v: any): assert v is string 
{
  if ( typeof v !== 'string' ) { 
    throw new AssertionError() 
  }
}

example code of a dynamic/templated type predicate:

cyclonedx-javascript-library/src/_helpers/notUndefined.ts

Lines 20 to 24 in 9367f5d

    
           export type NotUndefined<T> = T extends undefined ? never : T 
        
           export function isNotUndefined<T> (value: T | undefined): value is NotUndefined<T> { 
        
             return value !== undefined 
        
           }

lal12 · 2023-03-03T14:35:41Z

@jkowalleck
I already (partially) do that, e.g. function assertNonEmptyStr (value: any, path: PathType): asserts value is string { (here https://github.com/lal12/cyclonedx-javascript-library/blob/3e014d71c84d129e0384953f97493e8814771231/src/serialize/json/denormalize.ts#L45)

I still had it on my list to look into assertTypes (value: any, expected: VarType[], path: PathType): void, whether some kind of generic mapped assertion is possible (probably).

But I will refactor/change that anyway to get it working with warningFunc: (msg: string) => void.

Signed-off-by: Luca Lindhorst <[email protected]>

lal12 · 2023-03-03T16:00:57Z

@jkowalleck did some refactoring. Removed spec from factory and created a context object instead.

jkowalleck · 2023-03-03T16:14:47Z

@lal12
General question: why do you compare values against null - like value != null?
As far as I know, none of the CycloneDX JSON schemas has any nullable value.
They have optional, which would be undefined, not null -- so you should compare value !== undefined. But best would be to simply test for the correct type - skip the null checks ...

Signed-off-by: Luca Lindhorst <[email protected]>

lal12 · 2023-03-03T17:24:49Z

@jkowalleck

@lal12 General question: why do you compare values against null - like value != null? As far as I know, none of the CycloneDX JSON schemas has any nullable value. They have optional, which would be undefined, not null -- so you should compare value !== undefined. But best would be to simply test for the correct type - skip the null checks ...

Yeah doesn't really make sense anymore with the warning function. Corrected.

Fixed remaining type issues

lal12 · 2023-03-03T23:12:13Z

@jkowalleck something I've noticed while writing tests is that normalizer will look for BomRef by reference to the object in

cyclonedx-javascript-library/src/serialize/json/normalize.ts

Line 501 in 9367f5d

Array.from(deps).filter(d => allRefs.has(d) && d !== ref),

instead of by it's string value. Is this intended?

Made tests working, not ideal, but comparing boms fails due to URLs and loadNormalizeResult drops undefined, so I have to renormalize and stringify.

web build complains that there is no URL polyfill, should I add one?

Waiting for your review

jkowalleck · 2023-03-03T23:40:52Z

@jkowalleck something I've noticed while writing tests is that normalizer will look for BomRef by reference to the object in

cyclonedx-javascript-library/src/serialize/json/normalize.ts

Line 501 in 9367f5d

Array.from(deps).filter(d => allRefs.has(d) && d !== ref),

instead of by it's string value. Is this intended?

yes. is intended.
read https://github.com/CycloneDX/cyclonedx-php-library/blob/master/docs/dev/decisions/BomDependencyDataModel.md
Background: BomRef's must be unique. Therefore, the values are not final and will be discriminated - see https://github.com/CycloneDX/cyclonedx-php-library/blob/5f475ce38750745a45f920664ec1acaf0aa02b1a/src/Core/Serialization/BaseSerializer.php#L73-L74

web build complains that there is no URL polyfill, should I add one?

where? how? this is new.
do you have evidence/logs?

Waiting for your review

will review soon

lal12 · 2023-03-04T01:02:04Z

I import import {URL} from 'url'; which the web build doesn't like, without further config. Just gotta remove it.

jkowalleck · 2023-03-04T08:33:35Z

I import import {URL} from 'url'; which the web build doesn't like, without further config. Just gotta remove it.

just remove this import. URL is a predefined global class in JS.

jkowalleck

did not review all of it, stopped at a point.

Generally I see a big wrong concept here, which i will try to explain

you are deserializing the JOSN to some data, and force the data to be denormalized. while the denormalization is already ongoing, it is decided to stop all of this, because something seams odd.
THIS MUST NOT BE.
correct would be:
1. Deserialize the JSON to some data
2. check if this data is an object, that has the property "bomFormat" set to "CycloneDX" - if not STOP/THROW
3. check if this data is an object, that has the property "specVersion" set to "one value that is known"
4. fetch the Spec from SpecVersionDict based on the "specVersion" properrty. If it is not existing: Exit/Throw. If the Spec does not support JSON: Exit/Throw
5. feed the data model into the denormalizer.

very basic (pseudo) example code to show the process:

Deserializer {
  readonly #df
  constructor(df: DenormalizerFactory) {
    this.df=df
  }
  deserialize(bom: string) {
    const data = JSON.parse(bom)
    if (typeof data !== 'object') { throw RangeError('no object') }
    if (data.bomFormat !== 'CycloneDX') { throw RangeError('wrong format') }
    const spec = SpecVersionDict[data.specVersion]
    if (! spec?.supportsFormat('JSON')) { throw SpecVersionDict('something') }
    this.#df.makeForBom().denormalize(data)
  }
}
// i dont see a need to inject the spec into the normalizer.

src/serialize/json/denormalize.ts

jkowalleck · 2023-03-03T23:52:06Z

src/serialize/json/denormalize.ts

+    assertNonEmptyStr(url, path)
+    try {
+      return new URL(url)
+    } catch (e) {


error not used? then just write try{ ...} catch { ... } without keeping the error e

jkowalleck · 2023-03-03T23:54:33Z

src/serialize/json/denormalize.ts

+      version: warnStringOrUndef(data.version, ctx, [...path, 'version']),
+      tagVersion: warnNumberOrUndef(data.tagVersion, ctx, [...path, 'tagVersion']),
+      text: denormalizeRecord(data.text, ctx, [...path, 'text'], this._factory.makeForAttachment(ctx)),
+      url: url !== undefined


better turned, so that no bool switch is needed:

url === undefined ? url : this._factory.makeForUrl(ctx).denormalize(url, ctx, [...path, 'url'])

jkowalleck · 2023-03-03T23:56:20Z

src/serialize/json/denormalize.ts

+}
+
+function callWarnFunc (ctx: JSONDenormalizerContext, warning: JSONDenormalizerWarning): void | never {
+  if (typeof ctx.options.warningFunc !== 'function') {


turn arround. no bool inversin needed.

jkowalleck · 2023-03-05T08:53:46Z

src/serialize/types.ts

 export interface Serializer {
  /**
   * @throws {@link Error}
   */
  serialize: (bom: Bom, options?: SerializerOptions & NormalizerOptions) => string
 }
+
+export type VarType = 'string' | 'number' | 'bigint' | 'boolean' | 'symbol' | 'undefined' | 'object' | 'function' | '_array' | '_record'


these are no things we want to publish, they are internal.
need to find a place where to put this, so it is not exported to downstream

jkowalleck · 2023-03-05T08:57:19Z

src/serialize/jsonDeserializer.ts

+    data: any,
+    options: JSONDenormalizerOptions = {}
+  ): Bom {
+    return this.#normalizerFactory.makeForBom({ options })


options are injected into the class (constructor) and into the function? why???

jkowalleck · 2023-03-05T08:59:26Z

src/serialize/json/denormalize.ts

+}
+
+function warnStringOrUndef (value: unknown, ctx: JSONDenormalizerContext, path: PathType): string | undefined {
+  if (value !== undefined && typeof value !== 'string') {


turn bool compare around. and twist the code blocks.
value === undefined || typeof value === 'string'

jkowalleck · 2023-03-05T09:02:27Z

src/serialize/json/denormalize.ts

+import { Format, SpecVersionDict, UnsupportedFormatError } from '../../spec'
+import type { JSONDenormalizerOptions, JSONDenormalizerWarning, PathType } from '../types'
+
+interface JSONDenormalizerContext {


why is this needed? why are these three values encapsulated into a record?
i found rare usage - and none was justified. all usage i found was artificially complex.

jkowalleck · 2023-03-05T09:18:07Z

src/serialize/json/denormalize.ts

+  denormalize (data: Record<string, unknown>, { options }: { options: JSONDenormalizerOptions }, path: PathType): Models.Bom {
+    assertEnum(data.bomFormat, ['CycloneDX'], [...path, 'bomFormat'])
+    assertEnum(data.specVersion, Object.keys(SpecVersionDict), [...path, 'specVersion'])
+    const spec = SpecVersionDict[data.specVersion as keyof typeof SpecVersionDict] as Protocol


why is this as keyof typeof SpecVersionDict needed? the assertEnum should have done this already. if not, then it is done wrong.

i dont see a real need for this complex usage. please simplify

const spec = SpecVersionDict[data.specVersion as Version] if (!spec) { throw Error('something') }

jkowalleck · 2023-03-05T09:24:00Z

src/serialize/json/denormalize.ts

+    assertEnum(data.bomFormat, ['CycloneDX'], [...path, 'bomFormat'])
+    assertEnum(data.specVersion, Object.keys(SpecVersionDict), [...path, 'specVersion'])
+    const spec = SpecVersionDict[data.specVersion as keyof typeof SpecVersionDict] as Protocol
+    if (!spec.supportsFormat(Format.JSON)) {


❌ NO!
the (de)normalizer does not decide whether to run or not - it just does the job.
the (de)serializer might decide to not run because of constraints.

jkowalleck · 2023-04-04T11:45:14Z

FYI: i will be working on #620 soon.

first version of json deserialization

5390fa6

Signed-off-by: Luca Lindhorst <[email protected]>

jkowalleck requested changes Feb 23, 2023

View reviewed changes

jkowalleck changed the title ~~first version of json deserialization~~ [WIP] feat: JSON deserialization Feb 23, 2023

jkowalleck added the enhancement New feature or request label Feb 23, 2023

jkowalleck changed the title ~~[WIP] feat: JSON deserialization~~ [WIP] feat: JSON Deserializer/Denormalizer Feb 23, 2023

improved denormalize to be more flexible

3e014d7

Signed-off-by: Luca Lindhorst <[email protected]>

jkowalleck requested changes Mar 2, 2023

View reviewed changes

improved json denormalizer

5960d8c

Signed-off-by: Luca Lindhorst <[email protected]>

Luca Lindhorst added 2 commits March 3, 2023 18:21

fixed remaining typing issues

7e48069

Signed-off-by: Luca Lindhorst <[email protected]>

compare to undefined instead of null

11dd3be

Signed-off-by: Luca Lindhorst <[email protected]>

fixed tests, fixed dependencies

962e04e

lal12 marked this pull request as ready for review March 3, 2023 23:12

lal12 requested a review from a team as a code owner March 3, 2023 23:12

lal12 marked this pull request as draft March 3, 2023 23:13

jkowalleck self-requested a review March 4, 2023 08:30

removed URL import

db6cdfa

jkowalleck requested changes Mar 5, 2023

View reviewed changes

lal12 closed this Mar 6, 2023

jkowalleck mentioned this pull request Mar 7, 2023

feat: Deserializer/Denormalizer #86

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] feat: JSON Deserializer/Denormalizer #506

[WIP] feat: JSON Deserializer/Denormalizer #506

lal12 commented Feb 23, 2023 •

edited

Loading

jkowalleck Feb 23, 2023

jkowalleck Mar 2, 2023

jkowalleck commented Feb 23, 2023

lal12 commented Mar 2, 2023 •

edited

Loading

jkowalleck left a comment •

edited

Loading

jkowalleck Mar 2, 2023

jkowalleck commented Mar 3, 2023 •

edited

Loading

lal12 commented Mar 3, 2023 •

edited

Loading

lal12 commented Mar 3, 2023

jkowalleck commented Mar 3, 2023 •

edited

Loading

lal12 commented Mar 3, 2023

lal12 commented Mar 3, 2023 •

edited

Loading

jkowalleck commented Mar 3, 2023 •

edited

Loading

lal12 commented Mar 4, 2023 •

edited

Loading

jkowalleck commented Mar 4, 2023 •

edited

Loading

jkowalleck left a comment

jkowalleck Mar 3, 2023

jkowalleck Mar 3, 2023

jkowalleck Mar 3, 2023

jkowalleck Mar 5, 2023

jkowalleck Mar 5, 2023

jkowalleck Mar 5, 2023

jkowalleck Mar 5, 2023

jkowalleck Mar 5, 2023

jkowalleck Mar 5, 2023

jkowalleck commented Apr 4, 2023

[WIP] feat: JSON Deserializer/Denormalizer #506

[WIP] feat: JSON Deserializer/Denormalizer #506

Conversation

lal12 commented Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkowalleck commented Feb 23, 2023

lal12 commented Mar 2, 2023 • edited Loading

jkowalleck left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkowalleck commented Mar 3, 2023 • edited Loading

lal12 commented Mar 3, 2023 • edited Loading

lal12 commented Mar 3, 2023

jkowalleck commented Mar 3, 2023 • edited Loading

lal12 commented Mar 3, 2023

lal12 commented Mar 3, 2023 • edited Loading

jkowalleck commented Mar 3, 2023 • edited Loading

lal12 commented Mar 4, 2023 • edited Loading

jkowalleck commented Mar 4, 2023 • edited Loading

jkowalleck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkowalleck commented Apr 4, 2023

lal12 commented Feb 23, 2023 •

edited

Loading

lal12 commented Mar 2, 2023 •

edited

Loading

jkowalleck left a comment •

edited

Loading

jkowalleck commented Mar 3, 2023 •

edited

Loading

lal12 commented Mar 3, 2023 •

edited

Loading

jkowalleck commented Mar 3, 2023 •

edited

Loading

lal12 commented Mar 3, 2023 •

edited

Loading

jkowalleck commented Mar 3, 2023 •

edited

Loading

lal12 commented Mar 4, 2023 •

edited

Loading

jkowalleck commented Mar 4, 2023 •

edited

Loading