Skip to content
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

IQzone Bidder Adapter: add new bid param #1987

Merged
merged 7 commits into from
Sep 15, 2021

Conversation

IQZoneAdx
Copy link
Contributor

Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

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

Overall LGTM, added one minor comment

Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

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

LGTM

for i, imp := range request.Imp {
reqCopy.Imp = []openrtb2.Imp{imp}

if err = json.Unmarshal(reqCopy.Imp[i].Ext, &bidderExt); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be json.Unmarshal(reqCopy.Imp[0].Ext, &bidderExt)? Line 35 above you have defined reqCopy.Imp[] to have only one entry in its array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, thanks for finding this problem, we fixed it

hhhjort
hhhjort previously approved these changes Sep 13, 2021
Copy link
Collaborator

@hhhjort hhhjort left a comment

Choose a reason for hiding this comment

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

LGTM

for _, imp := range request.Imp {
reqCopy.Imp = []openrtb2.Imp{imp}

if err = json.Unmarshal(reqCopy.Imp[0].Ext, &bidderExt); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Declaring bidderExt and iqzoneExt at the start rather than in the loop, means they are persistent. This could potentially cause an issue if a field that exists in imp[0] is omitted from imp[1], the Unmarshal call will not zero out that parameter and thus you could see it where it does not belong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for finding this bug and explaining it. We have fixed this.

@hhhjort hhhjort dismissed their stale review September 14, 2021 18:00

Noticed a potential bug

Copy link
Collaborator

@hhhjort hhhjort left a comment

Choose a reason for hiding this comment

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

LGTM

@VeronikaSolovei9 VeronikaSolovei9 merged commit e129905 into prebid:master Sep 15, 2021
jizeyopera pushed a commit to operaads/prebid-server that referenced this pull request Oct 13, 2021
shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022
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