OCR-D / ocrd-webapi-implementation

4 stars 0 forks source link

Inconsistency in the models.py #19

Open MehmedGIT opened 1 year ago

MehmedGIT commented 1 year ago

The ProcessorRsrc model in models.py is extending the BaseModel it should rather extend Resource.

Resource(BaseModel)
ProcessorRsrc(BaseModel)
WorkspaceRsrc(Resource)
WorkflowRsrc(Resource)

From the openapi.yml spec:

Resource:
  type: object
  required: ['@id']
  properties:
    '@id':
      type: string
      description: URL of this thing
    description:
      type: string
      description: Description of the thing
Workspace:
  allOf:
    - {$ref: '#/components/schemas/Resource'}
Workflow:
  allOf:
    - {$ref: '#/components/schemas/Resource'}
Processor:
  description: The ocrd-tool.json for a specific tool
  x-$ref: 'https://ocr-d.de/ocrd_tool.schema.json#/properties/tools/patternProperties/ocrd-.*'

Since the Processor model does not have field id we cannot directly extend the Resource model.

I suggest the following correction in the openapi.yml spec:

Processor:
  id: ocrd-.*
  description: The ocrd-tool.json for a specific tool
  x-$ref: 'https://ocr-d.de/ocrd_tool.schema.json#/properties/tools/patternProperties/ocrd-.*'

Then this will be possible:

Resource(BaseModel)
ProcessorRsrc(Resource)
WorkspaceRsrc(Resource)
WorkflowRsrc(Resource)
joschrew commented 1 year ago

I think the issue belongs to https://github.com/OCR-D/spec. In my opinion the reasons are wrong: the spec should not be changed so the code can be improved, the spec should just care about if itself makes sense or not. I think the idea behind the processor-response is just to return the ocrd-tool.json. I don't know why one would need the ocrd-tool.json but I would think adding a/the id would change the output and a user gets something that is not expected. But I could be wrong of course

MehmedGIT commented 1 year ago

I already see an opened issue there. It is more general but in the same direction. The suggestion is not made just to improve the code, but the spec as well.