Re: [PATCH] JSON Schema Mapping property now works as expected

[Mohammad-Reza Nabipoor]

Hi, Kostas.

On Thu, Apr 01, 2021 at 05:59:54PM +0300, Konstantinos Chasialis wrote:
> Hello,
> 
> Recently we had a discussion with Mohammad on IRC about "Mapping" object
> on the JSON Schema.
> 
> We had an issue the following issue.
> 
> Suppose "mapped" was false (which means the object is not mapped), then
> "IOS", "strict" and "offset" did not need to be defined so the only truly
> "required" field was "mapped".
> 
> But this had a flaw. Because "maxProperties" were 4 and the only
> "required" property was "mapped" the user could easily define something
> like :
> 
> "PokeValue" : {
>     ...
>     "mapping" : {
>         "mapped": false,
>         "strict": true,
>         "IOS": 123,
>         "strict": false
>     }
> }
> 
> which is bad imo.
> 
> With the new changes, user is obliged to define "strict", "IOS", "offset"
> only if the field "mapped" is true.
> In case "mapped" is false he is not allowed to define anything else
> (or even define the "mapped" field twice).
> 

Nice improvement.
But I still think, as we discussed in IRC, we need to be more compatible
with `struct pvm_mapinfo` defined in `libpoke/pvm-val.h` file:

```c
struct pvm_mapinfo
{
  int mapped_p;
  int strict_p;
  pvm_val ios;
  pvm_val offset;
};
```

And the initializations in `libpoke/pvm-val.c`:

```c
  PVM_MAPINFO_MAPPED_P (arr->mapinfo) = 0;
  PVM_MAPINFO_STRICT_P (arr->mapinfo) = 1;
  PVM_MAPINFO_IOS (arr->mapinfo) = PVM_NULL;
  PVM_MAPINFO_OFFSET (arr->mapinfo) = pvm_make_ulong (0, 64);
```

```c
  PVM_MAPINFO_MAPPED_P (sct->mapinfo) = 0;
  PVM_MAPINFO_STRICT_P (sct->mapinfo) = 1;
  PVM_MAPINFO_IOS (sct->mapinfo) = PVM_NULL;
  PVM_MAPINFO_OFFSET (sct->mapinfo) = pvm_make_ulong (0, 64);
```

I think we should be compatible with the codes above.
Something like this:


```
        "Mapping": {
            "type": "object",
            "title": "Mapping",
            "properties": {
                "mapped": {
                    "type": "boolean",
                    "default": false
                },
                "strict": {
                    "type": "boolean",
                    "default": true
                },
                "IOS": {
                    "type": "object",
                    "oneOf": [
                        {
                            "type": "integer",
                            "minimum": -2147483648,
                            "maximum": 2147483647
                        },
                        {
                            "type": "null"
                        }
                    ],
                    "default": null
                },
                "boffset": {
                    "type": "object",
                    "allOf": [
                        { "$ref": "#/definitions/UnsignedInteger" }
                    ],
                    "properties": {
                        "size": {
                            "const": 64
                        }
                    },
                    "default": {
                        "PokeValue": {
                            "type": "UnsignedInteger",
                            "value": 0,
                            "size": 64
                        }
                    }
                }
            },
            "additionalProperties": false,
            "maxProperties": 4,
            "required": [
                "mapped",
                "strict",
                "IOS",
                "offset"
            ]
        },
```

> I know we do not have a validator right now and all of these are not
> really applicable but we might have one in the future.
> 


I think we cannot add a more dependency to the project, but we can validate
JSON schema on sample values manually before pushing to the master (or using
a git hook). 


> I also added the "default" property which I'd like to mention that it does
> not work as : "in case the user did not define this property, define it
> and give it this value".
> 
> Such thing does not exist in JSON Schema. The "default" property is like
> "title", just for documentation purposes.
> 


I think having such a field is useful.



Regards,
Mohammad-Reza