Skip to content

Updates from multiple PR reviews#930

Open
darinkrauss wants to merge 3 commits intodarin/update-responder-usagefrom
darin/review-updates
Open

Updates from multiple PR reviews#930
darinkrauss wants to merge 3 commits intodarin/update-responder-usagefrom
darin/review-updates

Conversation

@darinkrauss
Copy link
Copy Markdown
Contributor

  • Updates from multiple PR reviews

- Updates from multiple PR reviews
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR consolidates a set of review-driven updates across the codebase, primarily improving Oura integration (client/provider/API routing), test utilities, and serialization/config handling.

Changes:

  • Introduce an oura/service/api/v1 router (go-json-rest style) with accompanying tests and temporary glue for legacy data/service routes.
  • Add/expand Oura client/provider functionality and tests, including a typed 422 error response parser and new mock generation.
  • Update shared test utilities and serialization helpers (new ObjectFormatConfig, string array helpers, HTTP test response writer), plus a handful of correctness and consistency fixes.

Reviewed changes

Copilot reviewed 83 out of 83 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
work/test/work.go Omit metadata field when empty in test object generation.
work/processor.go Improve interface doc comments for clarity.
work/process_result.go Clarify exported helper comment.
work/base/processor.go Avoid encoding metadata when decode fails to prevent corruption.
user/test/user.go Use new string-array-to-[]any helper for roles serialization.
test/time.go Add ObjectFormatConfig time serialization support.
test/test.go Add gomock matcher adapter for Gomega matchers.
test/string_array.go Replace legacy string array object helper with cloning + []any array helper.
test/object.go Add ObjectFormatConfig.
test/int64.go Add config-format int64 serialization as string.
test/int.go Add config-format int serialization as string.
test/http/http.go Export HTTP methods/status codes lists; add ResponseWriter test helper.
test/float64.go Add config-format float64 serialization as string.
test/duration.go Add config-format duration serialization as string.
test/bool.go Add config-format bool serialization as string.
services/migrations/back-3857/back-3857-consent.go Replace Wrapf with Wrap where no formatting is used.
pointer/equal_test.go Update tests for renamed pointer equality helper.
pointer/equal.go Rename generic pointer equality helper to EqualValue.
platform/mutator.go Replace Wrapf with Wrap where no formatting is used.
oura/work/processors/processors_test.go Fix test description to match behavior.
oura/webhook/work/subscribe/processor_test.go Adjust tests for subscription expiration time now being a formatted string.
oura/webhook/work/subscribe/processor.go Parse expiration time from string and improve error messages for update/renew/create.
oura/test/oura.go Update Oura test generators/serializers for expiration time as string; improve randomness constraints.
oura/service/api/v1/v1_test.go Add tests for new v1 router dependencies and routes.
oura/service/api/v1/v1_suite_test.go Add Ginkgo suite bootstrap for v1 package.
oura/service/api/v1/v1.go Add v1 router + legacy glue wrappers for deprecated route style.
oura/service/api/v1/subscription_test.go Add handler tests for webhook subscription verification endpoint.
oura/service/api/v1/subscription.go Convert subscription endpoint to router method using rest request/response types.
oura/service/api/v1/event_test.go Add handler test for event endpoint response.
oura/service/api/v1/event.go Convert event endpoint to router method and return empty 200 response.
oura/provider/test/config.go Add test helpers for Oura provider config generation/serialization.
oura/provider/provider_test.go Add provider behavior tests (dependency validation, lifecycle, work creation/deletion).
oura/provider/provider_suite_test.go Add Ginkgo suite bootstrap for provider package.
oura/provider/provider.go Wire Oura client with new error parser; adjust error handling and data source/provider session updates.
oura/provider/config_test.go Add config loading/validation tests using config reporter + ObjectFormatConfig.
oura/provider/config.go Prevent partner secret from being JSON-serialized; simplify tags.
oura/oura_test.go Update tests for subscription expiration time type change (time -> string).
oura/oura.go Change Subscription.ExpirationTime to *string and validate via AsTime(...).
oura/client/test/error_parser.go Add random generators for Oura error response structures.
oura/client/test/client_mocks.go Add generated gomock mocks for Oura client provider.
oura/client/error_parser_test.go Add tests for new 422 error response parsing behavior.
oura/client/error_parser.go Implement typed error response parser with body size limiting + structured meta.
oura/client/client_test.go Add extensive Oura client request/response tests (subscriptions, personal info, revoke).
oura/client/client_suite_test.go Add Ginkgo suite bootstrap for client package.
oura/client/client.go Add mockgen directive, header constants, fix renew error wrapping message, and minor documentation tweaks.
oauth/work/mixin_test.go Update expectations for updated/expired booleans on downstream errors.
oauth/work/mixin.go Preserve updated/expired boolean on errors; reflect token state changes more accurately.
oauth/token/source.go Use IsExpiredAt(time.Now()) for expiry logic and restructure ExpireToken flow.
oauth/provider/test/provider.go Add helpers for generating alphanumeric client credentials.
oauth/provider/test/config.go Add oauth provider config test helpers + serialization helpers.
oauth/provider/config.go Prevent client secret JSON serialization; fix token URL load default.
oauth/provider/client/test/config.go Add oauth provider client config test helpers.
oauth/provider/client/config.go Inline provider config fields and make client config optional in JSON.
oauth/provider/client/client.go Minor naming cleanup (prvdr, clnt) for clarity.
log/test/serializer.go Improve assertion failure output by marshaling fields with fallback.
log/json/serializer.go Replace Wrapf with Wrap where no formatting is used.
dexcom/test/alert_schedule.go Update string-array serialization helper usage.
data/work/device_hashes_test.go Update parsing tests to support nil/empty cases using pointer helpers.
data/work/device_hashes.go Avoid nil deref and ensure map allocation during parse.
data/types/test/base.go Update string-array serialization helper usage.
data/types/settings/controller/test/device.go Update string-array serialization helper usage.
data/types/settings/cgm/test/scheduled_alert.go Update string-array serialization helper usage.
data/source/work/mixin.go Use pointer helper instead of taking address of field.
data/source/test/source.go Omit metadata field when empty in test object generation.
data/source/store/structured/mongo/mongo.go Use constants in index filter; align delete log/error messages.
data/set/work/mixin.go Clone pointer value instead of direct assignment.
data/raw/work/mixin.go Use pointer helper instead of taking address of field.
data/raw/test/raw.go Fix archivable/archived times and omit metadata when empty.
data/raw/store/structured/mongo/mongo.go Fix query construction for Archivable=false filter logic.
data/raw/raw_test.go Fix BSON expected format; update archivable tests for time-parameterized method.
data/raw/raw.go Rename to IsArchivableAt(tm) and adjust comparison logic.
customerio/work/event/processor.go Validate metadata before use; avoid nil derefs by passing metadata explicitly.
consent/service/consent.go Replace Wrapf with Wrap where no formatting is used.
client/test/config.go Add client config test helpers and object serialization.
blob/test/blob.go Update string-array serialization helper usage.
auth/token_test.go Update token expiry tests to use IsExpiredAt.
auth/token.go Add IsExpiredAt; adjust JSON/BSON tags and parsing approach.
auth/test/token.go Use alphanumeric token strings; update scope serialization helper usage.
auth/test/scope.go Add scope cloning helper and config-style scope serialization.
auth/service/service/service.go Replace Wrapf with Wrap where no formatting is used.
auth/service/service/client.go Add nil-context and input validation for CreateProviderSession.
auth/providersession/work/mixin.go Use pointer helper instead of taking address of field.
Makefile Make ginkgo bootstrap/generate targets conditional on TEST being set.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

@toddkazakov toddkazakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only nits, lgtm

@@ -100,9 +100,13 @@ func (s *Store) List(ctx context.Context, userID string, filter *dataRaw.Filter,
query["processedTime"] = bson.M{"$exists": *filter.Processed}
}
if filter.Archivable != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if filter.Archivable != nil {
if pointer.Default(filter.Archivable, false) {

Copy link
Copy Markdown
Contributor Author

@darinkrauss darinkrauss Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is constructed so that the Archivable filter is optional. If it is nil, then do not restrict the query one way or the other. If it is true or false, then only query for those that are archivable or not, respectively.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. I just think the nested if statements

if filter.Archivable != nil {
    if *filter.Archivable {

can be simplified to

if pointer.Default(filter.Archivable, false) {

or just collapsed to

if filter.Archivable != nil && *filter.Archivable{

Copy link
Copy Markdown
Contributor Author

@darinkrauss darinkrauss Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand. Here's how I see it. Here is the code currently:

	if filter.Archivable != nil {
		if *filter.Archivable {
			query["archivableTime"] = bson.M{"$exists": true, "$lt": now}
		} else {
			query["$or"] = bson.A{
				bson.M{"archivableTime": bson.M{"$exists": false}},
				bson.M{"archivableTime": bson.M{"$gte": now}},
			}
		}
	}

So, there are three use cases, when filter.Archivable is:

  1. nil - nothing is added to the query - the query ignores any archivable state (documents can be archivable or not)
  2. true - the query["archivableTime"] clause is added to the query - the query is limited to documents that are archivable
  3. false - the query["$or"] clause is added to the query - the query is limited to documents that are NOT achivable

What am I missing?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're not missing anything - I missed the else branch, sorry.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 86 out of 86 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

oauth/provider/client/config.go:1

  • json:\",inline\" is not supported by Go’s encoding/json (unknown tag options are ignored), so this will not inline fields as intended and will instead serialize under the default field name (e.g., \"Provider\"). If the goal is a flattened provider config, embed the provider config anonymously (no field name) or implement custom marshal/unmarshal (or revert to json:\"provider\").
    oura/service/api/v1/subscription.go:1
  • This logs the raw verificationToken, which is a shared secret used to validate webhook subscription requests. Logging it can leak credentials into logs. Prefer logging only non-sensitive context (e.g., token present/absent) or a one-way hash/truncated value; also consider avoiding logging the challenge value if it is treated as sensitive by the partner.
    pointer/equal.go:1
  • Renaming the exported function from Equal to EqualValue is a breaking change for any downstream packages importing pointer.Equal. To preserve compatibility, keep Equal as a wrapper that delegates to EqualValue (and deprecate it if desired), or provide a type alias/forwarder until callers migrate.
    test/http/http.go:1
  • Exporting mutable package-level slices allows external code/tests to modify them, which can introduce flaky behavior and data races (especially with parallel tests). Consider keeping these unexported and/or exposing accessor functions that return a copy to prevent mutation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 103 out of 103 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

page/pagination.go:122

  • Removing the exported First helper is a breaking change for any callers of page.First. If this is not intentionally a major-version change, consider restoring it (or keeping it as a deprecated wrapper around the new preferred approach) to avoid breaking downstream consumers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +5 to 6
func EqualValue[T comparable](a *T, b *T) bool {
if (a == nil) != (b == nil) {
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming the exported function Equal to EqualValue is a breaking API change for any downstream packages importing pointer. Consider keeping Equal as a thin wrapper (possibly deprecated) that forwards to EqualValue to preserve backwards compatibility.

Copilot uses AI. Check for mistakes.
@darinkrauss darinkrauss force-pushed the darin/review-updates branch 3 times, most recently from 22cee4b to 74a3128 Compare March 28, 2026 00:11
@darinkrauss darinkrauss force-pushed the darin/review-updates branch from 74a3128 to 2840ed9 Compare March 28, 2026 04:49
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

Successfully merging this pull request may close these issues.

3 participants