spec-json-schemas test: 3.0.0 initial tests

Changes:

Vitest instead of Mocha
schemas base tests
bindings tests

closes #539

May 22 '24 00:05 Pakisan

@Pakisan do you mind turning on all tests? Its completely fine if they fail as either it's the example that are incorrect or the JSON schema file

If failed tests didn't block release or merges, I'm ok with it

May 24 '24 07:05 Pakisan

If failed tests didn't block release or merges, I'm ok with it

Lets see whether it's the test that are wrong or the actual schemas first 🙂

May 24 '24 07:05 jonaslagoni

If failed tests didn't block release or merges, I'm ok with it

Lets see whether it's the test that are wrong or the actual schemas first 🙂

Let's merge it and solve skipped cases step by step

For example, Server example is not valid because serverVariable.examples is array, that's why:

Invalid

{
  "description": "This value is assigned by the service provider, in this example `gigantic-server.com`",
  "examples": null,
  "enum": null,
  "default": "demo"
}

Valid

{
  "description": "This value is assigned by the service provider, in this example `gigantic-server.com`",
  "default": "demo"
}

Because schema MUST be fixed, for examples, for example:

From this:

{
  "examples": {
    "type": "array",
    "description": "An array of examples of the server variable.",
    "items": {
      "type": "string"
    }
  }
}

To this:

{
  "examples": {
    "type": ["null", "array"],
    "description": "An array of examples of the server variable.",
    "items": {
      "type": "string"
    }
  }
}

Jun 02 '24 16:06 Pakisan

If failed tests didn't block release or merges, I'm ok with it

Lets see whether it's the test that are wrong or the actual schemas first 🙂

Let's merge it and solve skipped cases step by step

For example, Server example is not valid because serverVariable.examples is array, that's why:

Invalid
{
  "description": "This value is assigned by the service provider, in this example `gigantic-server.com`",
  "examples": null,
  "enum": null,
  "default": "demo"
}
Valid
{
  "description": "This value is assigned by the service provider, in this example `gigantic-server.com`",
  "default": "demo"
}
Because schema MUST be fixed, for examples, for example:

From this:
{
  "examples": {
    "type": "array",
    "description": "An array of examples of the server variable.",
    "items": {
      "type": "string"
    }
  }
}
To this:
{
  "examples": {
    "type": ["null", "array"],
    "description": "An array of examples of the server variable.",
    "items": {
      "type": "string"
    }
  }
}

But setting examples to null is not allowed in the spec? https://www.asyncapi.com/docs/reference/specification/v3.0.0#serverVariableObject

Only extensions and address (on channel) are allowed to be null.

Jun 03 '24 07:06 jonaslagoni

If failed tests didn't block release or merges, I'm ok with it

Lets see whether it's the test that are wrong or the actual schemas first 🙂

Let's merge it and solve skipped cases step by step For example, Server example is not valid because serverVariable.examples is array, that's why: Invalid
{
  "description": "This value is assigned by the service provider, in this example `gigantic-server.com`",
  "examples": null,
  "enum": null,
  "default": "demo"
}
Valid
{
  "description": "This value is assigned by the service provider, in this example `gigantic-server.com`",
  "default": "demo"
}
Because schema MUST be fixed, for examples, for example: From this:
{
  "examples": {
    "type": "array",
    "description": "An array of examples of the server variable.",
    "items": {
      "type": "string"
    }
  }
}
To this:
{
  "examples": {
    "type": ["null", "array"],
    "description": "An array of examples of the server variable.",
    "items": {
      "type": "string"
    }
  }
}
But setting examples to null is not allowed in the spec? https://www.asyncapi.com/docs/reference/specification/v3.0.0#serverVariableObject

Only extensions and address (on channel) are allowed to be null.

They are not required

Set them to null or avoid them is similar behavior for me and for serializers:

write properties with null values
don't write properties with null values

Current implementation tells that null is not allowed, which is not correct

Jun 03 '24 08:06 Pakisan

Current implementation tells that null is not allowed, which is not correct

@Pakisan do you mean current implementation of the JSON Schema files or?

If yes, then no, null are not allowed, there is a difference between a property being set and a null value 🙂

Jun 03 '24 08:06 jonaslagoni

So far I can see the following skipped tests (that need to be addressed):

AMQP 0.2.0 channel binding, 3 skipped test

AMQP 0.3.0 channel binding, 3 skipped test

SNS 0.1.0 operation binding, all tests skipped

Solace 0.2 operation binding, 2 test skipped

Solace 0.3 operation binding, 2 test skipped

Solace 0.4 operation binding, 2 test skipped

SQS 0.2 channel binding, 2 test skipped

Did I miss any @Pakisan?

Should we remove any old binding version tests? I.e. you both have test for AMQP 0.2 and 0.3, would it not be enough with 0.3 in this initial PR?

Skipped tests:

Operation Reply. uri-reference not compatible with #/components/...
Operation Trait Reason: errors with bindings, external docs, ...
Reference extended. Reason: schema doesn't check for extensions
...

I'll create discussion to review each case to understand is it wrong schema realization or not

Should we remove any old binding version tests? I.e. you both have test for AMQP 0.2 and 0.3, would it not be enough with 0.3 in this initial PR?

Nope, I don't want to remove old bindings. Furthermore I'll ask you to review proposal to add back MQTT5 to BindingsObject, also a to write common binding schema for all unimplemented bindings

I don't know why we decided to remove old versions from bindings repo, also as remove MQTT5 from bindings.

it's wrong decision - I'm using (maybe other folks) old bindings in old projects, and my specs are not valid anymore

Jun 03 '24 08:06 Pakisan

I'll create discussion to review each case to understand is it wrong schema realization or not

👍

Nope, I don't want to remove old bindings.

Okay 👍

Furthermore I'll ask you to review proposal to add back MQTT5 to BindingsObject, also a to write common binding schema for all unimplemented bindings

I don't know why we decided to remove old versions from bindings repo, also as remove MQTT5 from bindings.

it's wrong decision - I'm using (maybe other folks) old bindings in old projects, and my specs are not valid anymore

Wrong place for that discussion 🙂

Jun 03 '24 08:06 jonaslagoni

Current implementation tells that null is not allowed, which is not correct

@Pakisan do you mean current implementation of the JSON Schema files or?

If yes, then no, null are not allowed, there is a difference between a property being set and a null value 🙂

I don't sure that I got your point

Property with null is critical for Json Merge Patch(RFC 7386)

Null values in the merge patch are given special meaning to indicate the removal of existing values in the target.

In other cases it's ok to write or ignore property with null value to show that it doesn't contain any meaningful value

On deserialization you will interpret null value or missing property as scenario where it will be null or default value

Jun 03 '24 08:06 Pakisan

I don't sure that I got your point Property with null is critical for Json Merge Patch(RFC 7386)

In JSON there is a difference between allowing {examples: null} and not setting the property at all {}. {examples: null} according to the specification, is NOT allowed, so we cannot change that in the schema to be allowed, as you propose 🙂

In other cases it's ok to write or ignore property with null value to show that it doesn't contain any meaningful value

On deserialization you will interpret null value or missing property as scenario where it will be null or default value

Be careful about bringing Java-specific implementation details into a generic JSON schema validation library. I.e. yes in Java there is no difference between a null value and optional property, i.e. when you serialize an object it can come out as {examples: null}. However, the specification does not care about implementation details, only the correct JSON values.

I.e. {examples: null} is not allowed.

If you use something like Jackson for the serialization process, I have tried with something like this: https://www.baeldung.com/jackson-optional before to allow both null values and optional properties.

Property with null is critical for Json Merge Patch(RFC 7386)

That's a good point! However, it is beyond this discussion as that is a specification discussion on how to handle those cases where you want to remove existing properties when used in conjunction with traits 🤔

As of right now, only places null JSON values are allowed are extensions and address 🙂

Jun 03 '24 08:06 jonaslagoni

Here is ServerVariable class

@Data
@Builder
@NoArgsConstructor
@AllArgsConstructor
@EqualsAndHashCode(callSuper = true)
public class ServerVariable extends ExtendableObject {

    /**
     * An enumeration of string values to be used if the substitution options are from a limited set.
     */
    @Nullable
    @JsonProperty(value = "enum")
    private List<String> enumValues;

    /**
     * The default value to use for substitution, and to send, if an alternate value is not supplied.
     */
    @Nullable
    @JsonProperty("default")
    private String defaultValue;

    /**
     * An optional description for the server variable. <a href="https://spec.commonmark.org/">CommonMark syntax</a> MAY be used for rich text representation.
     */
    @Nullable
    private String description;

    /**
     * An array of examples of the server variable.
     */
    @Nullable
    private List<String> examples;

}

I don't use Optional<T> on class fields, cause they were designed for other purposes (args, for example), so I don't propose to implement language-specific mechanisms

In ServerVariable Json Schema here are not required, that's why me, generators, other folks can interpret it like allowance to avoid property or set it to null

In JSON there is a difference between allowing {examples: null} and not setting the property at all {}. {examples: null} according to the specification, is NOT allowed, so we cannot change that in the schema to be allowed, as you propose 🙂

Looks like you are talking about ECMAScript Language Specification, but Json (ECMA-404 The JSON Data Interchange Standard) says that:

However, it does not attempt to impose ECMAScript’s internal data representations on other programming languages. Instead, it shares a small subset of ECMAScript’s syntax with all other programming languages.

This interpretation of null as {} looks more like ECMA story

For Java and Go, for example:

We can't interpret {} as valid null, nil value on deserialization, out of the box
null(Java) or nil(Go) will be mapped to null on serialization

Go: https://pkg.go.dev/encoding/json

Pointer values encode as the value pointed to. A nil pointer encodes as the null JSON value.

Java: User can choose interpret null as no field in object, of field with null value

summary: now looks like language segregation, if your stack writes null, out of the box, choose other tools for specification

That's why I propose to move out this point and discuss it with other maintainers of our Json Schemas and call other folks from DX and Building Blocks working groups

@asyncapi/developer_experience_wg @asyncapi/essential_building_blocks_wg

Jun 03 '24 11:06 Pakisan

The only question at hand is, "what is a valid AsyncAPI document?" - It does not matter in what language, it's an indisputable truth, no matter the implementation 🙂 i.e. what is a valid AsyncAPI document in JSON and YAML defined as the format.

And even though we want the JSON Schema files to be that source of truth, it will be whatever is written in https://www.asyncapi.com/docs/reference/specification/v3.0.

asyncapi: 3.0.0
info:
  title: example with null and optional
  version: 1.0.0
servers:
  test:
    host: {test}.com
    protocol: http
    variables:
      test:
        examples: null

According to https://www.asyncapi.com/docs/reference/specification/v3.0.0#serverVariableObject (right this second) will never be considered valid.

asyncapi: 3.0.0
info:
  title: example with null and optional
  version: 1.0.0
servers:
  test:
    host: {test}.com
    protocol: http
    variables:
      test: {}

However is valid, because examples are an optional property.

Therefore it's not a question of whether a language outputs it like the invalid example, it will still be considered invalid and other tools are not expected to say it is.

If you want to enable null values i.e. the current invalid example above, it needs to be a feature request to https://github.com/asyncapi/spec 😄

Jun 03 '24 13:06 jonaslagoni

@jonaslagoni agree. Will create proposal to add null and create discussion to discuss current state of Schemas

What I need to improve, to merge tests as is and fix them step by step, binding each case with topic in discussion?

Jun 04 '24 13:06 Pakisan

@smoya fyi

Jun 05 '24 11:06 Pakisan

The Specification specifically states

examples | [string] | An array of examples of the server variable.

So not serializing it seems correct as a null array is not a valid example (by definition its NOT an example) as the Test variable is now also technically null (as it has no properties)

An array with a value that is null however, should be correct.

test:
  examples: null

vs

test:
  examples:
  - null

List<T> List = null != List<T> List = new List<T> { null } This appears more to be a fallacy with the Java (and Go) implementations leaning heavily on libraries for out of the box (de)serialization, rather than implementing the specification specifically.

How it works in the .net lib

(for writing) For the keen eyed, there is a version taking any struct (generally, numbers, dates etc.) which figures out what the behaviour needs to be depending on nullability and type.

Its the same pattern for objects and collections. return, {} or [] respectively.

For reading, its mainly validators that takes care of stating which required properties are missing (if any)

I dont see how serializing/deserializing optional null object and list properties to and from null will help. The specification is about communicating intent and usage; "Examples: null" doesn't communicate anything to anyone.

What even is a null example?

Trying to put it a bit into perspective examples: null == examples: [] == (no examples). So why serialize/deserialize if its an optional property. There is no error in not having it. Required properties is a different matter though, but here it should wholly depend on the type (collection [], object {}, scalar null)

Jun 05 '24 21:06 VisualBean

I would appreciate if you summarize what should I review from this PR. I see tons of kinda repeated tests and I just want to understand the expectation.

Jun 11 '24 17:06 smoya

@jonaslagoni @smoya

Basic test structure is next:

schema:
- empty
- example
- without required fields
- only required fields
- extended
- wrongly extended

Let's merge it. I'll extend tests under this issue #551

Jul 01 '24 11:07 Pakisan

LGTM :+1:

Can't find review, which is blocking merge, when approves appear

Jul 02 '24 16:07 Pakisan

@fmvilas @derberg @dalelane @smoya @char0n @GreenRover any of you want to have a quick look before merging it?

Jul 02 '24 17:07 jonaslagoni

Hi everybody,

One thing I've noticed are fixture files containing empty spaces. Example: ./without required properties.json

Wouldn't kebab-case or snake-case be more appropriate here?

I would also probably inquire why mocha was replaced with vitest? Was there any actual test case mocha couldn't handle?

There is also an unanswered comment from @smoya - https://github.com/asyncapi/spec-json-schemas/pull/540#issuecomment-2161251446. Answering it would help me as well.

Thanks for this huge work!

Jul 02 '24 18:07 char0n

Hi everybody,

One thing I've noticed are fixture files containing empty spaces. Example: ./without required properties.json

Wouldn't kebab-case or snake-case be more appropriate here?

I would also probably inquire why mocha was replaced with vitest? Was there any actual test case mocha couldn't handle?

There is also an unanswered comment from @smoya - #540 (comment). Answering it would help me as well.

Thanks for this huge work!

Hi!

I have summarized here - https://github.com/asyncapi/spec-json-schemas/pull/540#issuecomment-2199862506

New tests will look like this, so no files

import {describe} from 'vitest';
import {
    JsonSchemaTestSuite,
    JsonSchemaTestSuiteConfig,
    JsonSchemaTestSuiteData
} from '@test/definitions/base-schema-test.mjs';

const example = {
    "name": "AsyncApi",
    "url": "https://www.asyncapi.com",
    "email": "[email protected]"
}

const onlyRequiredProperties = {}

const withoutRequiredProperties = {}

const extended = {
    "name": "AsyncAPI",
    "url": "https://www.asyncapi.com",
    "email": "[email protected]",
    "x-number": 0,
    "x-string": "",
    "x-object": {
        "property": {}
    }
}

const wronglyExtended = {
    "name": "AsyncAPI",
    "url": "https://www.asyncapi.com",
    "email": "[email protected]",
    "x-number": 0,
    "x-string": "",
    "x-object": {
        "property" : { }
    },
    "ext-number": 1
}

const jsonSchema = require('@definitions/3.0.0/contact.json');

const config = new JsonSchemaTestSuiteConfig(
  false,
  [],
  true,
  []
);

const data = new JsonSchemaTestSuiteData(
  jsonSchema,
  [example],
  onlyRequiredProperties,
  withoutRequiredProperties,
  extended,
  wronglyExtended
);

describe('Contact', () => {
    new JsonSchemaTestSuite(data, config).testSuite()
});

I'll inject basic test resources to class. More specified cases will be stored nearly, in JSON, in future

I would also probably inquire why mocha was replaced with vitest? Was there any actual test case mocha couldn't handle?

Vitest and mocha are backward compatible. I switched to Vitest to introduce aliases like @test/definitions/base-schema-test.mjs instead of relative paths

Reference: https://github.com/Pakisan/spec-json-schemas/commit/03fde748a9bcc82b333d0825c66cde9f3945c6d4

Jul 02 '24 18:07 Pakisan

@asyncapi/bounty_team

Jul 24 '24 19:07 aeworxet

Quality Gate failed

Failed conditions
1 Security Hotspot
19.0% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Jul 26 '24 09:07 sonarqubecloud[bot]

@fmvilas @derberg @dalelane @smoya @char0n @GreenRover @jonaslagoni

Check out new tests

tldr;

95 test files for definitions and bindings instead of 400+, previously in this MR
common tests structure - empty, extended, wrongly extended, only required, without required, examples

Jul 26 '24 09:07 Pakisan

There is a lot here, so I'll likely have more comments once I've had a proper look through.

My initial reaction is mostly to the use of spaces in the file names for the new files - I realise this is very much a personal style/preference thing, so I'm sure it works fine as-is, but I would've preferred these to be hyphenated (such as the files in https://github.com/asyncapi/spec-json-schemas/tree/master/migrations and https://github.com/asyncapi/spec-json-schemas/tree/master/scripts ) or camel-cased (such as the files in https://github.com/asyncapi/spec-json-schemas/tree/master/definitions/3.0.0 )

I'll have a proper look and try and add some more substantive comments later