Alfresco / rest-api-explorer

Public REST API Explorer
Apache License 2.0
37 stars 25 forks source link

Properties field on person should accept structured data #116

Closed agologan closed 3 years ago

agologan commented 3 years ago

While testing on one of the trials environment we've noticed that properties field on person returns a dictionary with values being arrays of attributes which is different to the definition and ends up raising a parsing exception with bindings that include type information. As such this PR aims to remove the value type for the properties object on person introduced by https://github.com/Alfresco/rest-api-explorer/pull/99

gavincornwell commented 3 years ago

I would suggest checking with @eromano in that case, I doubt he would have added the definition for no reason so by removing you may effect the JS-API.

agologan commented 3 years ago

Added him as a reviewer for when he gets back.

eromano commented 3 years ago

The additional properties, in this case, should mean: https://swagger.io/docs/specification/data-models/dictionaries/ As I wrote that means that properties it's a map of string like from documentation:

{
  "en": "English",
  "fr": "French"
}

Looking the code of the model https://github.com/Alfresco/alfresco-remote-api/blob/master/src/main/java/org/alfresco/rest/api/model/Person.java the definition of properties is actually

    protected transient Map<String, Object> properties;

That object in a map of a model seems to be really flexible so it could be not only strings but that map is after populated with all strings rather than one value that seems to be a long that is the quota. Can you add here an example of a response with properties set that is creating problems?

agologan commented 3 years ago

Can you add here an example of a response with properties set that is creating problems?

Certainly. This is from one of the trials environments:

{
    "entry": {
        ...
        "properties": {
            "dynsc:dcp_371HjKIq": [
                "a4mAGCUA"
            ],
            "dynsc:dcp_cnbRqxRb": [
                "unmarked"
            ],
            "dynsc:dcp_dYwBooBg": [
                "4kr1hgf6"
            ]
        }
    }
}
eromano commented 3 years ago

yep the value here looks like an array, what are those values about? we should try to avoid having an object so generic as the value of a map

gavincornwell commented 3 years ago

IIRC when we originally put the OpenAPI spec together there wasn't support for declaring a map of string -> object hence the reason it was defined as just object.

Properties stored in the repository can have many different types hence the definition of Map<String, Object> so unless there is a way to specify that in OpenAPI unfortunately we'll have to stick to the vague definition of "object".

This may have been addressed in OpenAPI 3 which would give us another incentive to upgrade ;-)

eromano commented 3 years ago

so is it ok this modify right @gavincornwell ?