Skip to content

Change Regs.COPPA, Banner.TopFrame, Video.MinBitRate to pointer types - #29

Open
sujanchalla0510 wants to merge 1 commit into
prebid:mainfrom
sujanchalla0510:fix/zero-value-omitempty-fields
Open

Change Regs.COPPA, Banner.TopFrame, Video.MinBitRate to pointer types#29
sujanchalla0510 wants to merge 1 commit into
prebid:mainfrom
sujanchalla0510:fix/zero-value-omitempty-fields

Conversation

@sujanchalla0510

Copy link
Copy Markdown

Fixes #13.

Summary

Regs.COPPA, Banner.TopFrame, and Video.MinBitRate were typed as
plain int8/int64 with json:"...,omitempty". encoding/json's
omitempty treats a value type's zero value as empty, so explicitly
setting one of these fields to a meaningful 0 was indistinguishable,
once marshaled, from never having set the field at all:

  • Regs.COPPA: 0 means "not subject to COPPA" — a real,
    compliance-relevant signal, not "unknown".
  • Banner.TopFrame: 0 means "not in the top frame".
  • Video.MinBitRate: a real 0 Kbps floor is a valid value, per the
    spec.

All three were silently dropped from the marshaled JSON when set to
0, so a sender explicitly signaling "no"/"0" produced wire output
identical to never having set the attribute at all.

Fix

This repo already uses pointer types for the identical "0/1, omission
means Unknown" semantic on sibling fields in the same structs —
Regs.GDPR, Banner.Vcm, and Device.DNT — so this brings COPPA,
TopFrame, and MinBitRate in line with that existing, established
pattern rather than introducing a new one. Uses the existing
Int8Ptr/Int64Ptr helpers from ptr.go.

Breaking change

This is a source-breaking change for any caller directly assigning
these three fields as value types (e.g. banner.TopFrame = 1 becomes
banner.TopFrame = openrtb2.Int8Ptr(1)). Per the README this repo
follows semver with major-version releases for breaking changes (the
module is already at v20), so I'm assuming that's the expected path
here — happy to adjust if you'd prefer a different approach.

Test plan

  • Added zero_value_fields_test.go, which marshals each field set
    explicitly to 0 and asserts it survives in the output. Verified
    these tests fail to even compile against the old value types
    (proving the fix is necessary), and pass against the fix.
  • go build ./..., go vet ./..., go test ./... all pass.
  • gofmt clean.

These fields were typed as plain int8/int64 with `json:"...,omitempty"`.
encoding/json's omitempty treats a value type's zero value as empty, so
explicitly setting one of these fields to a meaningful 0 was
indistinguishable, once marshaled, from never having set the field:

  - Regs.COPPA: 0 means "not subject to COPPA" - a real, often
    compliance-relevant signal, not "unknown".
  - Banner.TopFrame: 0 means "not in the top frame".
  - Video.MinBitRate: a real 0 Kbps floor is a valid value.

All three were silently dropped from the marshaled JSON when set to 0,
so a sender explicitly signaling "no"/"0" produced wire output
identical to never having set the attribute at all.

This repo already uses pointer types for the identical "0/1, omission
means Unknown" semantic on sibling fields in the same structs -
Regs.GDPR, Banner.Vcm, and Device.DNT - so this brings COPPA, TopFrame,
and MinBitRate in line with that existing, established pattern rather
than introducing a new one. Uses the existing Int8Ptr/Int64Ptr helpers
from ptr.go.

Adds zero_value_fields_test.go, verified to fail to even compile against
the old value types and pass against the fix.

Fixes prebid#13
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.

Attributes (banner.TopFrame, regs.Coppa, video.MinBitRate) with int8 and omitempty

1 participant