papinet / papiNet-API

papiNet is a global paper, forest products and bioproducts industry e-Business initiative.
http://www.papinet.org/
Apache License 2.0
9 stars 3 forks source link

Responses that contains reference to something MUST only contain the identifier of that something #71

Closed patricekrakow closed 1 year ago

patricekrakow commented 2 years ago

I need to remove properties in the "/check-availability" sample responses!

{
  "sellerProducts": [
    {
      "id": "e7bfd8a6-edde-48ab-b304-b7d4f1d007a6",
      "name": "Galerie Brite",
      "paper": {
        "finishType": "Gloss",
        "printType": "HeatsetOffset",
        "basisWeight": {
          "value": 54,
          "UOM": "GramsPerSquareMeter"
        },

basisWeight MUST be specified as it has multiple possible values within the seller-product definition, while finishType MUST NOT be specified, as it has a single value within the seller-product definition.

patricekrakow commented 2 years ago

The responses within the Catalogue use case must also be adapted.

patricekrakow commented 2 years ago

Warning: This might be in contradiction with: "If the customer is not absolutely sure that the seller will not change the properties of this seller-product, then all the other important properties, such as finishType and printType MUST also be copied to the customer-article definition." (source: https://github.com/papinet/papiNet-API/blob/master/1.2.0/catalogue.md).

larsolofsson commented 1 year ago

Has this issue an impact on ### Rule 6 - Avoid lookup optimization? E.g in an order there is a reference to to seller-Product. Should all properties be specified? If not, what happens if the seller has changed the seller-Product since it was earlier requested by the customer?

patricekrakow commented 1 year ago

In order to minimise the size of the response payload (rule 8), we SHOULD NOT repeat paper properties that are fixed (only one value) within the seller-product definition. If there is a need to know such a property, the GET /seller-products/{sellerProductId} API endpoint can be used.

patricekrakow commented 1 year ago

Fixed in https://github.com/papinet/papiNet-API/commit/6fda4b1fa7d2effa02e97d2d709c3174df07f5f3.

patricekrakow commented 1 year ago

We also need to change the schema accordingly, e.g. the name property should be there but not required. The CheckAvailabilityOfSellerProductById schema cannot directly use the GetSellerProductById schema! We also need to change the CheckAvailabilityOfCustomerArticleById schema accordingly. We also need to update the responses in the Pact file accordingly.

patricekrakow commented 1 year ago

The Pact file has been updated, see https://github.com/papinet/papiNet-API/commit/c701d21283f42fbe766585329d5707c3a60a0185.

Regarding the changes in the schemas, it would be "stupid" (i.e. difficult to maintain) to copy paste the entire GetSellerProductById structure into the CheckAvailabilityOfCustomerArticleById just to remove the required constraint on the name property, as well as on the format property within the Paper structure (which would be even more "stupid" to copy). In JSON schema, you cannot remove constraints, but you can add them. Then, the idea is to start from a common structure called SellerProduct and to add the required constraint(s) using allOf. Please find below a little prototype of the technique that can be tested with https://www.jsonschemavalidator.net/:

{
  "$schema": "http://json-schema.org/draft-00/schema#",
  "type": "object",
  "properties": {
    "getSellerProductById": {
      "$ref": "#/GetSellerProductById"
    },
    "checkAvailabilityOfSellerProductById": {
      "$ref": "#/CheckAvailabilityOfSellerProductById"
    }
  },
  "GetSellerProductById": {
    "allOf": [
      { "$ref": "#/SellerProduct" },
      { "required": [ "id", "name" ] }
    ]
  },
  "CheckAvailabilityOfSellerProductById": {
    "allOf": [
      { "$ref": "#/SellerProduct" },
      { "required": [ "id" ] }
    ]
  },
  "SellerProduct": {
    "type": "object",
    "properties": {
      "id": {
        "type": "string"
      },
      "name": {
        "type": "string"
      }
    }
  }
}

together with this little instance:

{
  "getSellerProductById": {
    "id": "1",
    "name": "My 1st"
  },
  "checkAvailabilityOfSellerProductById": {
    "id": "2",
  }
}
patricekrakow commented 1 year ago

While the allOf technique above was working fine, we needed to be sure that it could be combine with the oneOf / anyOf technique of the issue #60! Here is a little prototype of the two techniques combined that can be tested with https://www.jsonschemavalidator.net/:

{
  "$schema": "http://json-schema.org/draft-00/schema#",
  "type": "object",
  "properties": {
    "getSellerProductById": {
      "$ref": "#/GetSellerProductById"
    },
    "checkAvailabilityOfSellerProductById": {
      "$ref": "#/CheckAvailabilityOfSellerProductById"
    }
  },
  "GetSellerProductById": {
    "allOf": [
      { "$ref": "#/SellerGenericProduct" },
      { "required": [ "id", "name" ] },
      { "oneOf": [
          {
            "required": [ "paper"],
            "properties": {
              "paper": {
                "allOf": [
                  { "$ref": "#/Paper" },
                  { "required": [ "format" ]}
                ]
              }
            }
          },
          {
            "required": [ "pulp"],
            "properties": {
              "pulp": {
                "$ref": "#/Pulp"
              }
            }
          }
        ]
      }
    ]
  },
  "CheckAvailabilityOfSellerProductById": {
    "allOf": [
      { "$ref": "#/SellerGenericProduct" },
      { "required": [ "id" ] },
      { "anyOf": [
          {
            "properties": {
              "paper": {
                "$ref": "#/Paper"
              }
            }
          },
          {
            "properties": {
              "pulp": {
                "$ref": "#/Pulp"
              }
            }
          }
        ]
      }
    ]
  },
  "SellerGenericProduct": {
    "type": "object",
    "properties": {
      "id": {
        "type": "string"
      },
      "name": {
        "type": "string"
      }
    }
  },
  "Paper": {
    "type": "object",
    "properties": {
      "name": {
        "type": "string"
      },
      "format": {
        "type": "string",
        "enum": [ "Reel", "Sheet" ]
      }
    }
  },
  "Pulp": {
    "type": "object"
  }
}

together with this little instance:

{
  "getSellerProductById": {
    "id": "1",
    "name": "My 1st",
    "paper": {
      "format": "Reel"
    }
  },
  "checkAvailabilityOfSellerProductById": {
    "id": "2"
  }
}
patricekrakow commented 1 year ago

We will implement the last changes in the schemas needed to fix this issue together with the changes needed to to fix the issue #60.

patricekrakow commented 1 year ago
  1. Remove the required constraint on the format property in the Paper schema.
  2. Add the Pulp schema as a placeholder for now.
  3. Reorganize the structure around seller-product as follow:
SellerGenericProduct  (id , ..., name , ..., descriptions)
^ GetSellerProductById(id*, ..., name*, ..., descriptions, (paper|pulp)*)
^ CheckAvailabilityOfSellerProductById
  ^ sellerProducts*   (id*, ..., name , ..., descriptions, paper|pulp)
  1. Reorganize the structure around customer-article as follow:
CreateCustomerGenericArticle(     ..., name , ..., descriptions)
^ CreateCustomerArticle     (     ..., name*, ..., descriptions, (paper|pulp)*)
^ CustomerGenericArticle    (id , ..., name , ..., descriptions)
  ^ GetCustomerArticleById  (id*, ..., name*, ..., descriptions, (paper|pulp)*)
  ^ CheckAvailabilityOfCustomerArticleById
    ^ customerArticles*     (id*, ..., name , ..., descriptions, paper|pulp)
patricekrakow commented 1 year ago

Fixed in https://github.com/papinet/papiNet-API/commit/7328d62b841083d679ce958e06f31df86b03162e.