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

Schemas are broken #1474

Open
jl-wynen opened this issue Nov 1, 2024 · 6 comments
Open

Schemas are broken #1474

jl-wynen opened this issue Nov 1, 2024 · 6 comments

Comments

@jl-wynen
Copy link
Contributor

jl-wynen commented Nov 1, 2024

The different dataset schemas are inconsistent with each other. As I understand it, you are in the process of changing them. But the current broken versions have been released and so affect Scitacean.

  • The dataset class now uses lists for instrumentIds, proposalIds, sampleIds. But the creation DTOs still use the old scalar instrumentId, proposalId, samplesId, where the derived dataset only has proposalId.
  • Investiagor:
    • Classs: principalInvestigator
    • Raw DTO: investigator and principalInvestigator
    • Derived DTO: investigator
  • Type: An enum in the class and a plain string in the DTOs. Used to be an enum everywhere. Given that it is going to be more flexible, a plain string is probably best.

Can the above be fixed easily and quickly? Scitacean is currently broken and needs to be updated. But that requires a lot of manual hacks to account for the above inconsistencies. I would prefer to not do this since it would be temporary anyway.

@sbliven
Copy link
Contributor

sbliven commented Nov 1, 2024

It looks like #1414 may have been merged prematurely and still hasn't been cleaned up. I'm a bit concerned that none of the tests revealed all the places where schema changes were not propagated. For instance, src/datasets/datasets.service.spec.ts still uses the old scalar properties, but the test passes and lints. VSCode flags some errors, but they don't seem to result in CI failures.

@nitrosx
Copy link
Contributor

nitrosx commented Nov 5, 2024

Being the author of PR #1414, I will check and fix it.

@nitrosx
Copy link
Contributor

nitrosx commented Nov 5, 2024

@jl-wynen the dataset class (aka the database representation of the dataset) correctly has only the principalInvestigator.
Raw DTO should only have principalInvestigator.
By the way, are you talking about create, update or output DTO?

To the SciCat user, the dataset type it is a string. Currently we only have raw and derived datasets. In the immediate future, site admins could add additional types to meet the needs of their institution.

@jl-wynen
Copy link
Contributor Author

jl-wynen commented Nov 5, 2024

By the way, are you talking about create, update or output DTO?

Create. I did not check the others.

@nitrosx
Copy link
Contributor

nitrosx commented Nov 7, 2024

PR #1487 should address this issue

@jl-wynen
Copy link
Contributor Author

Only partially.

  • CreateRawDatasetObsoleteDto still has both principalInvestigator and investigator.
  • The ids are the same scalars that they used to be in the schema. But the API returns the new array fields. So the generated models are wrong.

What is the prupose of OutputDatasetObsoleteDto? Is it even used anywhere?

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

3 participants