-
-
Notifications
You must be signed in to change notification settings - Fork 11
[BACK-3925] Refactor twiist data ingestion to find TidepoolLinkID via ProviderSession #889
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
base: master
Are you sure you want to change the base?
Conversation
darinkrauss
commented
Aug 6, 2025
- Refactor twiist data ingestion to find TidepoolLinkID via ProviderSession
- Refactor ListUserProviderSessions to ListProviderSessions
- Add UserID to ProviderSessionFilter
- Add LastDataSetID to Source
- Update Abbott submodule
- Update dependencies to resolve vulnerabilities
- Update Go version to resolve vulnerabilities
- https://tidepool.atlassian.net/browse/BACK-3925
… ProviderSession - Refactor twiist data ingestion to find TidepoolLinkID via ProviderSession - Refactor ListUserProviderSessions to ListProviderSessions - Add UserID to ProviderSessionFilter - Add LastDataSetID to Source - Update Abbott submodule - Update dependencies to resolve vulnerabilities - Update Go version to resolve vulnerabilities - https://tidepool.atlassian.net/browse/BACK-3925
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The biggest issue that I see is that we're potentially allowing data ingestion to a disconnected source. Let me know if I'm missing something.
@@ -16,9 +16,9 @@ import ( | |||
|
|||
func (r *Router) ProviderSessionsRoutes() []*rest.Route { | |||
return []*rest.Route{ | |||
rest.Get("/v1/users/:userId/provider_sessions", serviceApi.RequireServer(r.ListUserProviderSessions)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep this for backward compatibility or is it only used by other platform services?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this endpoint required a server token (i.e. serviceApi.RequireServer
) I did a search through all of the backend code for provider_sessions
(and downstream usages, specifically ListUserProviderSessions
) and it was not used anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new endpoint ListProviderSessions
is used now by the twiist data ingestion code in the data service to communicate with the auth service.
|
||
dataSources, err := dataServiceContext.DataSourceClient().ListAll(ctx, filter, nil) | ||
// Find matching data source | ||
dataSourceFilter := &dataSource.Filter{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that the filter for the state has been removed, perhaps by accident. I think we should check that the data source is in connected state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, the data source is found through the provider session and it must be in a connected state (otherwise it is a logic error). I guess the question here is what to do about it if the logic error happens:
- Log the inconsistency,
- Force the data source state to connected,
- Disconnect the provider session and update the data source to remove the providerSessionId.
I did a database query and didn't find any in this state on any environment. So, I think I'll just log the inconsistency as a warning.
func (s *Source) LastDataSetID() *string { | ||
if s.DataSetIDs == nil { | ||
return nil | ||
} else if length := len(*s.DataSetIDs); length < 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} else if length := len(*s.DataSetIDs); length < 1 { | |
} else if length := len(*s.DataSetIDs); length == 0 { |
7e99100
to
76888a6
Compare