-
Notifications
You must be signed in to change notification settings - Fork 732
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
PulsePoint: Marking cp/ct params to be either String or Int #3677
PulsePoint: Marking cp/ct params to be either String or Int #3677
Conversation
Code coverage summaryNote:
pulsepointRefer here for heat map coverage report
|
"type": [ | ||
"integer", | ||
"string" | ||
], | ||
"description": "An ID which identifies the publisher selling the impression" | ||
}, | ||
"ct": { | ||
"type": "integer", | ||
"type": [ | ||
"integer", | ||
"string" | ||
], |
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.
@anand-venkatraman could you add JSON test where cp
and ct
are string
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.
@anand-venkatraman requesting to address above comment ^^
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.
sorry for the delay, i have added a few tests cases to validParams
under params_test.go
file.
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.
@anand-venkatraman thanks for adding params
test. Requesting to add Json framework tests as well for scenario where cp
and ct
are string in imp.ext
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.
addressed in commit 4539af1
Code coverage summaryNote:
pulsepointRefer here for heat map coverage report
|
Code coverage summaryNote:
pulsepointRefer here for heat map coverage report
|
openrtb_ext/imp_pulsepoint.go
Outdated
PubID interface{} `json:"cp"` | ||
TagID interface{} `json:"ct"` |
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.
PubID and TagID are expected to be int or string.
However interface{}
will allow int, string and any other values
Should use StringInt
type
prebid-server/util/jsonutil/stringInt.go
Lines 1 to 29 in a1f48b5
package jsonutil | |
import ( | |
"github.com/buger/jsonparser" | |
) | |
type StringInt int | |
func (st *StringInt) UnmarshalJSON(b []byte) error { | |
if len(b) == 0 { | |
return nil | |
} | |
if b[0] == '"' { | |
b = b[1 : len(b)-1] | |
} | |
if len(b) == 0 { | |
return nil | |
} | |
i, err := jsonparser.ParseInt(b) | |
if err != nil { | |
return err | |
} | |
*st = StringInt(i) | |
return nil | |
} |
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.
updated params to use StringInt
instead of interface{}
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.
one comment - https:/prebid/prebid-server/pull/3677/files#r1655235030
Code coverage summaryNote:
pulsepointRefer here for heat map coverage report
|
Code coverage summaryNote:
pulsepointRefer here for heat map coverage report
|
Code coverage summaryNote:
pulsepointRefer here for heat map coverage report
|
No description provided.