Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistency in the models.py #19

Open
MehmedGIT opened this issue Nov 21, 2022 · 2 comments
Open

Inconsistency in the models.py #19

MehmedGIT opened this issue Nov 21, 2022 · 2 comments

Comments

@MehmedGIT
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants