Add a Role entity and relations from InstrumentScientist and InvestigationUser

[stuartpullinger]

Add a Role table with label and pid and use it for InstrumentScientist and InvestigationUser. This will also require two more tables InstrumentScientistRole and InvestigationUserRole to allow one InstrumentScientist or InvestigationUser to have many roles. This implies removing the role field in the InvestigationUser and InvestigationGroup.

This solution was agreed at a meeting of the ICAT collaboration on 18th August 2021. This solution would replace the solution in #237.

[RKrahl]

A comment on this, because I raised the question in today's meeting, without having carefully read the proposal before: if you add a many-to-many relation between InvestigationUser and Role (e.g. adding InvestigationUserRole), my concern about removing the role attribute from the uniqueness constraint in InvestigationUser is resolved. I was concerned because we have users with multiple roles in one investigation. But this will still be possible.

The only issue that I can see now is that the database upgrade script for this will be far from trivial: at the moment, we have multiple InvestigationUser entries between a user and an investigation having different role values. The upgrade script will need to merge them into a single one, e.g. retain one InvestigationUser, populate the Role table with multiple related entries accordingly and finally delete all the others. Putting that into a correct piece of SQL sounds tricky.

[kevinphippsstfc]

Now that we are looking at the ICAT schema changes more closely and planning our upgrades to it, we have realised that for Diamond in particular this change could potentially cause us problems in the near future due to a temporary lack of effort available to work on the User Office to ICAT propagation component which would need modifying.

What Diamond do need however in the near future is the schema changes for DataPublication. So I can see two possible options here:

1. We delay this Role change until a future release. (Can somebody confirm who needs this change and when they would need it by please?)
2. We keep the role attribute in InvestigationUser for now (as well as adding the new relationship to InvestigationUserRole) but mark it as deprecated such that it will be removed in a future release. This would allow facilities to be more flexible about when they migrate from using the role attribute to using the new relationship.

Does anybody have any preferences or comments on this?

@antolinos I'm mentioning you because you requested this change in #237 but it doesn't look like you are connected to this issue yet.

[antolinos]

@kevinphippsstfc No problem from my side to delay

[RKrahl]

I believe option 2, keeping the role attribute in InvestigationUser for a transition period, while adding the new Role and InvestigationUserRole tables at the same time, will be difficult in practice. The problem is that role is part of the uniqueness constraint in InvestigationUser and needs to be as long as we don't make the transition to the separate Role table, because we have users with more than one role in a given investigation.

To illustrate with an example: consider an investigation Investigation(name=inva) and a user User(name=userx). That user is at the same time proposer and member of the experimental team. As a result, we have two entries in the InvestigationUser table:

Investigation(name=inva) <- InvestigationUser(role=Proposer) -> User(name=userx)
Investigation(name=inva) <- InvestigationUser(role=Experimentator) -> User(name=userx)

If we remove role from the uniqueness constraint in InvestigationUser, we must merge these two entries at the same time and populate the Role table accordingly. As long as we keep role in the uniqueness constraint, we must set that attribute, because it is constraint to NOT NULL, so we cannot merge the two entries. (Well, of course we could merge them an set role=dummy, but that would be awkward.) So, we can't have one schema during a transition period that works well for both models at the same time.

I'd therefore prefer option 1 to delay that change. Since it is a breaking change, it would need to go in major version bump, e.g. to a future release 6.0. As a matter of fairness towards @antolinos, we should promise to envision that release for not too far in the future.

[kevinphippsstfc]

Thanks @RKrahl for clarifying the issue. Indeed it seems we are just left with the delaying option and I agree that we should provide support towards a releasing a version containing it not too far in the future, if @antolinos is OK with that. Hopefully at STFC and Diamond, we will be in a better position to do that soon.