-
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
ImproveDigital: Bad-Input Error #3469
Conversation
Code coverage summaryNote:
improvedigitalRefer here for heat map coverage report
|
Code coverage summaryNote:
improvedigitalRefer here for heat map coverage report
|
@@ -276,7 +276,7 @@ func (a *ImprovedigitalAdapter) getAdditionalConsentProvidersUserExt(request ope | |||
} | |||
|
|||
// Check key exist user.ext.ConsentedProvidersSettings.consented_providers | |||
var cpMap = make(map[string]json.RawMessage) | |||
var cpMap = make(map[string]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.
Could you please add a json test case for this change.
@@ -276,7 +276,7 @@ func (a *ImprovedigitalAdapter) getAdditionalConsentProvidersUserExt(request ope | |||
} | |||
|
|||
// Check key exist user.ext.ConsentedProvidersSettings.consented_providers | |||
var cpMap = make(map[string]json.RawMessage) | |||
var cpMap = make(map[string]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.
@ishihanvcs any particular reason to use interface{}
here? this change add step for asserting cMapValue to string (line 291)
consentStr := cpMapValue.(string)
On line 291, adapter code is not checking whether cpMapValue can be asserted into string. This could led to panic at run time. Refer https://go.dev/play/p/BnmZ2ZGjT3E as example
Additionally json.RawMessage
does gurantee that value parsed in cpMap
will be a valid json which can be converted further into string. But with interface{}
cpMap can have any value
if len(cpStr) == 0 { | ||
return nil, nil | ||
} | ||
//cpStr = consentStr[:len(consentStr)-1] |
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.
could remove this code comment
@onkarvhanumante Thanks for pointing out the issue precisely. As an occasional go developer, I was too hasty to look forward all possible execution path. I'll soon revert Also, I'll add a supplemental test for the multi ~ case in the value of |
Code coverage summaryNote:
improvedigitalRefer here for heat map coverage report
|
@onkarvhanumante @gargcreation1992 Code updated. Please review & merge if found okay. |
EDIT: Forward fix for prebid/prebid-server-java#2942
Original issue is found & reported on pbs java. As the pbs go adapters are continuously being ported to java version, we want to fix this here first and then provide fix in the java version.
Please review & merge kindly at your earliest convenience.
TIA