Privacy Sandbox: Topics in headers

[pm-nilesh-chate]

Implements #3008

[AlexBVolcy]

Looks good, left two very minor comments.

[pm-nilesh-chate]

The spec mentions that invalid entires should result in an error, but are ignored in this implementation.

The specification includes requirement 3.viii "If any validation fails, that header field should be ignored with a warning when in debug mode". That is not implemented in this PR. To implement, warnings should pass along the error chain to be filtered by the top level call to setFieldsImplicitly. Please try to match error wording from PBS-Java (if they have this implemented already).

@SyntaxNode Logging in setFieldsImplicitly for only debug mode is a bit tricky. Debug mode status is available in HoldAuction(). I would have to add warning logs (with new debug severity) in setFieldsImplicitly and remove these logs using errortypes.WithoutDebugWarnings(errs) while create response.ext inmakeExtBidResponse if debug is not enabled. Let me know if this approach is ok or do you suggest any other

[SyntaxNode]

@SyntaxNode Logging in setFieldsImplicitly for only debug mode is a bit tricky. Debug mode status is available in HoldAuction(). I would have to add warning logs (with new debug severity) in setFieldsImplicitly and remove these logs using errortypes.WithoutDebugWarnings(errs) while create response.ext inmakeExtBidResponse if debug is not enabled. Let me know if this approach is ok or do you suggest any other

I think it's better to emit warnings without checking the debug flag than emit no warnings at all. We can add the debug descriptor in the future if it's needed.

IMHO it's ok to use the existing warning functionality. Look at how errors are handled by deps.validateRequest to see how to pass warnings into HoldAuction for inclusion in the response. (which isn't ideal, but it's the way things are right now)

If you want to follow the debug requirement, I don't think you need to create a new severity level. The severity is primarily used to know when to end the auction early and how to classify the error in the response. I think we may be ok with adding a signal to the Coder interface and adding a WarningDebugOnly helper to filter out debug scoped warnings when debug is false.

[pm-nilesh-chate]

@SyntaxNode please review the latest changes. I'll add the UT, etc once the approach is accepted.

[SyntaxNode]

Thank you for updating the PR to include warning. I appreciate that you want to fully implement the debug condition.

Adding a new severity level is opening us up to potential bugs. There is a good amount of code which assumes an error is either fatal or a warning, but now these cases need to be expanded to include debug warnings. Some of the code pathways may be missed (we're only human, after all). I also expect this would be a large effort to test.

The only difference between a warning and a debug warning is when to write it to the response. All other code pathways are unchanged.

I recommended adding a property of some kind to the warning type which will identify it's scope. You only need to change the warning loop in exchange.go - which introduces less risk and requires less testing.

Thinking about it more, consider adding a scope.go in the errortypes package which looks something like this:

type Scope int

const (
  ScopeAny
  ScopeDebug
)

type Scoped interface {
  Scope() Scope
}

func ReadScope(err error) Scope {
  if e, ok := err.(Scoped); ok {
    return e.Scope()
  }
  return ScopeAny
}

When looping through errors, if debug is not enabled ignore any warnings with the ScopeDebug scope. You can still create a DebugWarning type with it implementing the Scoped interface and returning a ScopeDebug value. The default will be treat warnings as we do today.

[pm-nilesh-chate]

@SyntaxNode Thanks for the prompt review. Could you please review the latest commit which implements the recommended changes

[SyntaxNode]

@SyntaxNode Thanks for the prompt review. Could you please review the latest commit which implements the recommended changes

Looks good to me. Thank you for acting on this feedback.

New comments

[AlexBVolcy]

Just found some things related to code coverage I wanted to ask about, and a spelling nitpick.

[SyntaxNode]

Thank you for the updates. Looking good to me. I performed end-to-end testing via Postman. I found it difficult to setup a test via the Chrome browser itself. I used different test topics posted by Google and they all worked well.

A few small nitpicks before I approve.