fix: move runNumber in legacy scientificMetada to top by minottic · Pull Request #2609 · SciCatProject/backend

minottic · 2026-03-10T09:00:59Z

Description

Since runNumber was moved from scientificMetadata to a top level dataset property #1485 , this PR adds a migration for legacy data and a fallback when posting or patching v3 datasets which moves scientificMetadata.runNumber to runNumber. This 2nd change is to facilitate customers' adoption as they would need to change their ingestions.

Closes #2592

Tests included

Included for each change/fix?
Passing?

Documentation

swagger documentation updated (required for API changes)
official documentation updated

official documentation info

src/datasets/dto/update-dataset-obsolete.dto.ts

migrations/20260309161001-move-run-number-to-top.js

Junjiequan · 2026-03-11T10:01:00Z

migrations/20260309161001-move-run-number-to-top.js

+          filter: { _id: dataset._id },
+          update: {
+            $set: { "scientificMetadata.runNumber": dataset.runNumber },
+            $unset: {runNumber: ""},


Does this script addresses the following runNumber format?

"runNumber" : { "value" : NumberInt(482), "unit" : "" },

is anyone doing that? runNumber is simply an integer in the top level schema so I guess noone did that? I think we would have a hard time trying to imagine how everyone could have used runNumber in scientificMetadata, but if you reckon this shape is something they used, I can add to the migration

also, what should the migration do if there are unit? Simply concat?

I just checked our DB and some scientificMetadata contains the format above.
This change does not affect our side, as we are going to use high level run number anyways.
But I'm not sure would it affect others or not

ok, happy to add that!

nitrosx · 2026-03-11T11:04:27Z

I am not oppose to this migration script but it can only be optional.
Even if we have an high level field for run number, it is only a best practice to use this field.
We cannot assume what a facility decides to do with run number: use the high level field or store it in a scientific metadata entry.

Also it is a facility decision if they want to migrate legacy data or not and how to convert it from quantity (if they have it as quantity) to a numeric field.

minottic · 2026-03-11T11:13:30Z

I am not oppose to this migration script but it can only be optional.
Even if we have an high level field for run number, it is only a best practice to use this field.
We cannot assume what a facility decides to do with run number: use the high level field or store it in a scientific metadata entry.

Also it is a facility decision if they want to migrate legacy data or not and how to convert it from quantity (if they have it as quantity) to a numeric field.

the migration will run only if there are datasets with scientificMetadata.runNumber. I am not sure I understand the comment fully, doesn't this apply to all migrations then? This is a change in behaviour between the old backend and the new one, so a migration is needed. For example the FE runNumber in the datasets overview used to rely on scientificMetadata.runNumber and now fetches the top level run number. People will always be able to use scientificMetadata.runNumber in the v3 endpoints and this will move it to the topLevel runNumber. One could argue why this field was moved to a top level field without checking the old behaviour, but that's another question...

nitrosx · 2026-03-11T11:27:25Z

I am not oppose to this migration script but it can only be optional.
Even if we have an high level field for run number, it is only a best practice to use this field.
We cannot assume what a facility decides to do with run number: use the high level field or store it in a scientific metadata entry.
Also it is a facility decision if they want to migrate legacy data or not and how to convert it from quantity (if they have it as quantity) to a numeric field.

the migration will run only if there are datasets with scientificMetadata.runNumber. I am not sure I understand the comment fully, doesn't this apply to all migrations then? This is a change in behaviour between the old backend and the new one, so a migration is needed. For example the FE runNumber in the datasets overview used to rely on scientificMetadata.runNumber and now fetches the top level run number. People will always be able to use scientificMetadata.runNumber in the v3 endpoints and this will move it to the topLevel runNumber. One could argue why this field was moved to a top level field without checking the old behaviour, but that's another question...

The migration should run only if the facility decided to do so.
It is not up to SciCat project to force a facility to use high level field runNumber, although we can suggest it as best practice.
If a facility has datasets with scientific metadata entry named runNumber, we can not assume that they want to migrate and the migration makes sense.
I will be happy to discuss in person

nitrosx

This change, although technical and syntactically correct, needs more community discussion

src/datasets/dto/output-dataset-obsolete.dto.ts

minottic · 2026-03-11T11:31:46Z

The migration should run only if the facility decided to do so.

but why not for all migrations then? Also, noone forces anyone to run migrations, they are not run on startup. We do the same exact thing here

Say I am a user and I have a look at the POST datasets DTO. I will see a runNumber field. So I will add my runNumber there. Then all historical metadata used to use scientificMetadata.runNumber. If no migration I will have inconsistent behaviour

If you think we should not standardise how to use runNumber as a top level element, I think this will create a lot of confusion for the use, but I can manage that from my side

minottic · 2026-03-11T12:06:51Z

just to summarise the change, given the latest commit:

on PATCH/POST on v3 endpoint runNumber is populated from scientificMetadata.runNumber if runNumber is missing. Both are kept (we might think to remove the scientificMetadata.runNumber in this case but I am fine keeping it)
on GET on v3 scientificMetadata.runNumber is populated from runNumber if missing

since the migration is a source of discussion, I am happy to manage it from my side and not have it in this repo

minottic · 2026-03-17T09:57:02Z

any further reason this is blocked @nitrosx ?

minottic · 2026-03-19T09:24:28Z

@nitrosx , @Junjiequan any news here?

Junjiequan

Looks good to me

src/datasets/dto/output-dataset-obsolete.dto.ts

Junjiequan · 2026-03-20T11:22:47Z

If this PR is urgent we can dismiss review from @nitrosx , as he will mostly not available untill the week after next week.
I don't see any other blockers with this PR

Requested changes has been addressed. I don't see any other blockers. If later we find any blockers we can revert the PR by then

minottic · 2026-03-20T13:11:46Z

If this PR is urgent we can dismiss review from @nitrosx , as he will mostly not available untill the week after next week.
I don't see any other blockers with this PR

If you are happy with the changes, I would indeed proceed with the merge

Junjiequan · 2026-03-20T14:45:40Z

I just thought about one potential issues here. if we allow overwritting runNumber from scientificMetadata.runNumber if not exists, what about old datasets? wouldn't it causing incosistent datasets?
For example, when creating or updating a dataset, dataset.runNumber may not be provided, but the user might set scientificMetadata.runNumber for a different purpose than the actual meaning of runNumber.

Still need more discussion

minottic · 2026-03-20T14:55:23Z

I just thought about one potential issues here. if we allow overwritting runNumber from scientificMetadata.runNumber if not exists, what about old datasets? wouldn't it causing incosistent datasets?

That's exactly why we will run a migration at PSI. I think it's indeed not a trivial matter, because:

the FE consumes top level runNumber. Before it used to consume scientificMetadata.runNumber. Already this, kinda supports the argument that they have the same meaning
If I want runNumber to be displayed in FE I need to use the top level field
Even if I did not want the FE to use this field, any new user who would see the DTO of Datasets will put runNumber as a top level field, as that's the most intuitive thing if having a look at the schema (they would not think they need to use the scientificMetadata.runNumber)
Given that we have legacy users and data, we will very likely have historic users using "scientificMetadata.runNumber" and new users using "runNumber"
This will create inconsistencies
To overcome this, PSI will run a migration of its data moving scientificMetadata.runNumber to runNumber and the change of this PR would keep new data consistent

I am happy with other solutions but this bug is causing complains from our users.

Also, I honestly think asking ourselves "what if the user meant something by leaving runNumber empty and used scientificMetadata.runNumber" is a sensible question, but in reality a bit too theoretic, I believe anyone seeing runNumber would interpret it in the same way as scientificMetadata.runNumber and simply leave one of the 2 out (likely the nested one as it's not in the DTO)

Last point, when we moved scientificMetadata.runNumber to runNumber not only we caused this possible inconsistence, but we also introduced a breaking change, as any client would not know anymore what field to consume.

Junjiequan · 2026-03-20T15:07:22Z

After this change I'm afraid It still remains the root issue that what if user’s intent for scientificMeata.runNumber is not meant to be used for dataset.runNumber.

minottic · 2026-03-20T15:09:04Z

sorry, I updated the comment here quite a bit

minottic · 2026-03-20T15:14:51Z

and also, we've done these defaultings a lot of time it's done in publishedData DTOs and job DTOs.

But maybe to be pragmatic, what if I make another PR where I expose conditionally runNumber as a top level field? In this way, PSI could opt out from having it as a top level field and keep the old behaviour or I can make the change in this PR configurable via ENV?

Junjiequan · 2026-03-20T15:23:50Z

Is the complaining comes from mainly for FE display ? if it's the case the table config accepts scientificMetadata.runNumber.value and displays like so:

also within labelLocalization the label can be renamed to runNumber

minottic · 2026-03-20T15:26:38Z

no, the main problem will be this

Even if I did not want the FE to use this field, any new user who would see the DTO of Datasets will put runNumber as a top level field, as that's the most intuitive thing if having a look at the schema (they would not think they need to use the scientificMetadata.runNumber)

Junjiequan · 2026-03-20T15:26:56Z

In this PR I think the output part looks fine to me, my only concern was the create/update dto part which tries to predict/ twist user’s intent that it assumes dataset.scientificMeatadata.runNumber = dataset.runNumber

Junjiequan · 2026-03-20T15:50:14Z

If I understand the whole situation corretctly, the issue is that legacyusers or services that dont want to use or see the top level runNumber , as it can be confusing which one to use.

if that's the case, please go ahead and add env variables within this PR and expose the field conditionally. And we can treat top level runNumber as a new feature in v4

minottic · 2026-03-20T15:53:23Z

In this PR I think the output part looks fine to me, my only concern was the create/update dto part which tries to predict/ twist user’s intent that it assumes dataset.scientificMeatadata.runNumber = dataset.runNumber

I think the change cannot happen only in the get dto though, we have dashboarding tools that query the db directly, I'd like to avoid querying for both fields in each query.

But that's valuable feedback, I'll propose something different next week

nitrosx

@minottic apologies for being so demanding!!!
can you add a configuration variable in order to enable or disable this behavior?

minottic requested a review from a team as a code owner March 10, 2026 09:00

minottic force-pushed the run_scientific branch from da48c89 to e9a134d Compare March 10, 2026 09:12

fix: move runNumber in legacy scientificMetada to top

66c6208

minottic force-pushed the run_scientific branch from e9a134d to 66c6208 Compare March 10, 2026 10:22

bpedersen2 reviewed Mar 10, 2026

View reviewed changes

src/datasets/dto/update-dataset-obsolete.dto.ts Outdated Show resolved Hide resolved

migrations/20260309161001-move-run-number-to-top.js Outdated Show resolved Hide resolved

migrations/20260309161001-move-run-number-to-top.js Outdated Show resolved Hide resolved

Junjiequan reviewed Mar 11, 2026

View reviewed changes

nitrosx previously requested changes Mar 11, 2026

View reviewed changes

src/datasets/dto/output-dataset-obsolete.dto.ts Show resolved Hide resolved

minottic and others added 3 commits March 11, 2026 13:45

Add runNumber to v3 output DTOs

ecabab7

Avoid modifying scientificMetadata

54736d0

Remove migration

f233b25

minottic force-pushed the run_scientific branch from dc1f63b to f233b25 Compare March 11, 2026 13:45

minottic requested a review from nitrosx March 17, 2026 09:56

minottic mentioned this pull request Mar 17, 2026

Dataset's runNumber inconsistencies #2592

Open

Junjiequan previously approved these changes Mar 19, 2026

View reviewed changes

src/datasets/dto/output-dataset-obsolete.dto.ts Show resolved Hide resolved

minottic added 2 commits March 20, 2026 11:59

Change order of properties in transformation logic

52c958d

Merge branch 'master' into run_scientific

060adc3

nitrosx requested changes Mar 20, 2026

View reviewed changes

minottic marked this pull request as draft March 26, 2026 09:35

Conversation

minottic commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Tests included

Documentation

official documentation info

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Junjiequan Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

minottic Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minottic Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Junjiequan Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

minottic Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

nitrosx commented Mar 11, 2026

Uh oh!

minottic commented Mar 11, 2026

Uh oh!

nitrosx commented Mar 11, 2026

Uh oh!

nitrosx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

minottic commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

minottic commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

minottic commented Mar 17, 2026

Uh oh!

minottic commented Mar 19, 2026

Uh oh!

Junjiequan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Junjiequan commented Mar 20, 2026

Uh oh!

minottic commented Mar 20, 2026

Uh oh!

Junjiequan commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

minottic commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Junjiequan commented Mar 20, 2026

Uh oh!

minottic commented Mar 20, 2026

Uh oh!

minottic commented Mar 20, 2026

Uh oh!

Junjiequan commented Mar 20, 2026

Uh oh!

minottic commented Mar 20, 2026

Uh oh!

Junjiequan commented Mar 20, 2026

Uh oh!

Junjiequan commented Mar 20, 2026

Uh oh!

minottic commented Mar 20, 2026

Uh oh!

nitrosx left a comment

Choose a reason for hiding this comment

Uh oh!

minottic commented Mar 10, 2026 •

edited

Loading

minottic Mar 11, 2026 •

edited

Loading

minottic commented Mar 11, 2026 •

edited

Loading

minottic commented Mar 11, 2026 •

edited

Loading

Junjiequan commented Mar 20, 2026 •

edited

Loading

minottic commented Mar 20, 2026 •

edited

Loading