Extract FPD Merge To Separate Package #3533
Conversation
| } | ||
|
|
||
| // user memory protect test | ||
| func TestMergeUserMemoryProtection(t *testing.T) { |
There was a problem hiding this comment.
Removed the memory protection tests entirely in favor of updating the main tests.
| "github.com/prebid/prebid-server/v2/util/jsonutil" | ||
| ) | ||
|
|
||
| func App(v *openrtb2.App, overrideJSON json.RawMessage) (*openrtb2.App, error) { |
There was a problem hiding this comment.
Since Go requires a package name in the function invocation, this will always be used as merge.App which looks nice IMHO. Thoughts?
| originalApp := ortb.CloneApp(&test.givenApp) | ||
| merged, err := App(&test.givenApp, test.givenJson) | ||
|
|
||
| assert.Equal(t, &test.givenApp, originalApp) |
There was a problem hiding this comment.
This is my attempt at rolling in the memory protection tests. Clones the given request and verifies it hasn't changed after the call to the merge methods. Need a sanity check on this approach by those reviewing.
There was a problem hiding this comment.
Checked, app site and user look right!
firstpartydata/first_party_data.go
Outdated
| return nil, err | ||
| } else { | ||
| newUser = mergedUser |
There was a problem hiding this comment.
When err != nil evaluates to true we end up returning nil in both branches, do we really need mergedUser as a separate variable? We could even spare the else clause:
201 if fpdConfigUser != nil {
202 - if mergedUser, err := merge.User(newUser, fpdConfigUser); err != nil {
+ if newUser, err := merge.User(newUser, fpdConfigUser); err != nil {
203 if err == merge.ErrBadOverride {
204 return nil, ErrBadFPD
205 }
206 return nil, err
207 - } else {
208 - newUser = mergedUser
209 }
210 }
firstpartydata/first_party_data.go
But I see this pattern repeated in other places below (mergedSite and mergedApp) should we modify to make it leaner? Also, why do merge.User, merge.Site and merge.App are now returning pointers? Is it to check for shared memory issues?
There was a problem hiding this comment.
But I see this pattern repeated in other places below (mergedSite and mergedApp) should we modify to make it leaner?
Yes. My original intent was to not assign mergedUser if there is an error, but if there is an error there is no return path for newUser. Updated.
Also, why do merge.User, merge.Site and merge.App are now returning pointers?
They always did this. Before, they changed the memory address of the input pointer. Now it returns a separate pointer and leaves the input alone.
Pulling out this code for potential reuse and to isolate this behavior from FPD logic. Changed the function signature to be a pure method, but is functionality identical.
Realized some of the clone methods are expensive and most are likely not necessary for the merge. This will get worse as I look to introduce an imp merge function as the imp structure is much deeper.
I'm going to investigate a new merge approach which will only clone data present in the json override. Need to benchmark if the lazy cloner would be overall more efficient. Certainly would produce a lot less allocations, just need to verify the CPU cost isn't much higher.