SciCatProject / SciCatProject/backend

HistoryClass not type safe in typescript/python SDKs

Open
#2,293 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

v3-compatibility
Dominant language
TypeScript
Stars
27
Forks
37
Avg merge
17h 32m
Merged PRs (30d)
38

Description

Summary

HistoryClass is supposed to model the backend-v3 style history of Datasets, where the changed fields are placed at the top level:

{
    id: "...",
    updatedAt: "...",
    updatedBy: "...",

    keywords: {
        previousValue: [],
        currentValue: ["tag1"]
    },
    datasetlifecycle: {
        previousValue: {x: "a"},
        currentValue: {x: "b"}
    }
}

To accomodate this, an index signature [key: string]: unkown was added in the class. However, this has no representation in the OpenAPI document that is generated. i.e. the openapi generator does not e.g. generate additionalProperties: true for the HistoryClass schema.

As a result, various SDK generators, which take as input the OpenAPI document, only generate a class with the mandatory history fields i.e.

{
    id: "...",
    updatedAt: "...",
    updatedBy: "...",
}

e.g. historyClass.d.ts. And we need to manually intersect HistoryClass with the index signature in the frontend e.g.

export type HistoryWithProperties = HistoryClass & {[key:string]: unknown}

to get proper type checking.

In the python sdk, trying to add an extra field to the HistoryClass instance triggers a runtime error:
see discussion here.

Steps to Reproduce

Current Behaviour

Expected Behaviour

Quting a propsed fix from the above discussion:
I think a fix to the HistoryClass would be to move the additional properties in a dedicated field e.g. changes, whose type is Record<string, unknown>, e.g. in the UserSettings model, this way OpenAPI contains this information:

  "externalSettings": {
      "type": "object",
      "default": {},
      "additionalProperties": true,
}

and pydantic generates

external_settings: Dict[str, Any] = Field(...

and the typescript sdk contains:

    externalSettings: {
        [key: string]: any;
    };

Then changes could be made to the frontend / response of /v3/datasets to still present the output in the old format.

Details

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with src/datasets/schemas/history.schema.ts and compare its index signature with the UserSettings model in src/users/schemas/user-settings.schema.ts. Inspect the generated OpenAPI schema and the TypeScript and Python SDK output, then trace the /v3/datasets response. Done means the history changes are represented in OpenAPI and generated SDKs while the existing response format remains supported.

Written by the indexing model from the issue text.

Assessment

Tech stack
openapi, python, typescript
Domain
api, backend, backend-api-design
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.