Patch Timeseries group office permission issues

[Enovotny]

closed issue #879 and opened this one since it is really a different issue.

When patching a time series group owned by CWMS but with timeseries owned by a district office I get a user not authorized. I had to give myself write permissions to the CWMS office in order to get past that error. I don't think that is correct it should be checking the permission on the assigned-time-series office-id. At least that is they way it works in CWMS-Vue. Users don't need write permission to CWMS but to the time series office-ID.

Once I gave myself permission to the CWMS office I get a

cwms.cda.api.errors.NotFoundException: ORA-20025: LOCATION_ID_NOT_FOUND: The Location: "Westhope" does not exist.

But Westhope does exist for MVP. I wonder if it is checking CWMS office instead of MVP for this.

curl -X 'PATCH'
'https://cwms-data-test.cwbi.us/cwms-data/timeseries/group/SHEF%20Data%20Acquisition?replace-assigned-ts=false&office=CWMS'
-H 'accept: /'
-H 'Authorization: apikey '
-H 'Content-Type: application/json'
-d '{
"office-id": "CWMS",
"id": "SHEF Data Acquisition",
"time-series-category": {
"office-id": "CWMS",
"id": "Data Acquisition"
},
"assigned-time-series": [
{
"office-id": "MVP",
"timeseries-id": "Westhope.Stage.Inst.15Minutes.0.CEMVP-GOES-Raw",
"alias-id": "DE32E520.HG.RZZ.0015:TZ=UTC;DLTime=false;Units=ft"
}
]
}'

Also the request body in swagger needs to be updated it shows

"assigned-time-series": [
{
"officeId": "string",
"timeseriesId": "string",
"tsCode": 0,
"aliasId": "string",
"refTsId": "string",
"attribute": 0
}

It should be

"assigned-time-series": [
{
"office-id": "string",
"timeseries-id": "string",
"ts-code": 0,
"alias-id": "string",
"refTsId": "string", -> not sure what this should be.
"attribute": 0
}

[zack-rma]

Work plan to address this issue:

Add verification to TimeSeriesGroup Patch and LocationGroup Patch that the rename operation will not occur on CWMS owned groups, only updating the assigned TS/Locations.
Verify that no-op is the outcome for attempting to rename a CWMS owned group.

Create integration tests to confirm the oldGroupId is being properly decoded before reaching the rename operation. This could be causing the "location does not exist" error.

Review JSON and DAO's to see whether the correct office is being used in all update calls.

Update the TimeSeries DTO annotations to Kebab-case the assigned TimeSeries list.

@rma-psmorris

[rma-psmorris]

@zack-rma -- if you could also document the workflow of how the incoming assigned set is stored to the database given there can be deletes, inserts, and updates to the assignment table.

[zack-rma]

Assigned TS/Location Set Workflow:

The current setup for updating stored assigned Timeseries/Location values in the database starts with the TimeSeriesGroup/LocationGroup endpoints.

There are two methods by which to update the assigned groups: adding new values/updating existing values, or replacing all assigned TS/Locations for the specified group.

To add new values or update existing values, the TimeSeriesGroup/LocationGroup Patch can be used with the "replace assigned" flag set to false. This will assign these TS/Locations to the specified group. If the assignment already exists, the record associated with the group will be updated in the at_ts_group_assignment or at_loc_group_assignment table.

To replace/update the assigned TimeSeries/Location for a group, the same endpoint must be used with the "replace assigned" flag set to true. This will first unassign all assigned TS/Locations associated with the specified group. Then, the provided assignments will be associated with the group. In effect, all values are overwritten. This necessitates that any assigned TS/Location that the user wishes to keep assigned to the group must be included in the query.

[rma-psmorris]

@zack-rma , what are the specific database API calls made by the DAO in order of operations to process the set's inserts, updates, deletes and when does the database commit occur.

[zack-rma]

The following are in order of operation:

If the DAO renames the TS/Location Group (group id has changed + office is not CWMS):
- calls CWMS_TS_PACKAGE.call_RENAME_TS_GROUP

If the "unassign all" flag is set:
- calls CWMS_TS_PACKAGE.call_UNASSIGN_TS_GROUP

Finally, the DAO calls CWMS_TS_PACKAGE.call_ASSIGN_TS_GROUPS

I believe a database commit occurs with each PL/SQL call.

[MikeNeilson]

It would commit with each call, and even if you forced out a Connection to auto commit false and shared it there's an unfortunately possibility that the internal database code would have a commit in it.

What office is the connection set to when looping through the assignments or just calling assign_ts_groups?

[zack-rma]

The current default for these calls is the office provided in the input data. For the rename TS group call, that would be the office provided by the user which is associated with the TS group.

I've done a bit of testing with CWMS_SEC_PACKAGE.call_GET_USER_OFFICE_ID using the connection to grab the DB office, with some success on avoiding the authorization errors that were being thrown when trying to assign/unassign TS ids from a CWMS group.

[rma-psmorris]

It would commit with each call, and even if you forced out a Connection to auto commit false and shared it there's an unfortunately possibility that the internal database code would have a commit in it.

By design none of the PL/SQL API calls in the CWMS database call commit. Unless that design has been changed without announcement/discussion in our DB meetings.

[MikeNeilson]

By design none of the PL/SQL API calls in the CWMS database call commit. Unless that design has been changed without announcement/discussion in our DB meetings.

That should be correct. It sadly is not. But you're right that it's correct and current design guideless. Just not as well implemented as it should be.

[rma-psmorris]

By design none of the PL/SQL API calls in the CWMS database call commit. Unless that design has been changed without announcement/discussion in our DB meetings.

That should be correct. It sadly is not. But you're right that it's correct and current design guideless. Just not as well implemented as it should be.

Oh, that's interesting. One of the reasons the design was originally put in place was to allow external actors to be able to do rollbacks. I'll have to take a look in the DB to see where commits are done that would prevent that.

[MikeNeilson]

I know @DanielTOsborne got bit by at least one. I think it had to do with locations or something.