Refactor AMP Param Parsing

[SyntaxNode]

We're starting to separate amp specific functionality into it's own package, with the goal of creating slim endpoints funcs. Parsing is indirectly tested by pre-existing AMP tests. I'll need to add proper coverage before converting from a draft.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[SyntaxNode]

Removing stale label to end of year holidays.

[hhhjort]

LGTM

[guscarreon]

We can probably simplify makeFormatReplacement a bit more if we write a function that returns whatever non-zero value it finds:

423   func makeFormatReplacement(size amp.Size) []openrtb.Format {
424       var formats []openrtb.Format 
    +     width := getNonZeroInt(size.OverrideWidth, size.Width)
    +     height := getNonZeroInt(size.OverrideHeight, size.Height),
    +     if width != -1 && height != -1 {
    +         formats = []openrtb.Format{{
    +             W: width,
    +             H: height,
    +         }}
    +     }
425 -     if size.OverrideWidth != 0 && size.OverrideHeight != 0 {
426 -         formats = []openrtb.Format{{
427 -             W: size.OverrideWidth,
428 -             H: size.OverrideHeight,
429 -         }}
430 -     } else if size.OverrideWidth != 0 && size.Height != 0 {
431 -         formats = []openrtb.Format{{
432 -             W: size.OverrideWidth,
433 -             H: size.Height,
434 -         }}
435 -     } else if size.Width != 0 && size.OverrideHeight != 0 {
436 -         formats = []openrtb.Format{{
437 -             W: size.Width,
438 -             H: size.OverrideHeight,
439 -         }}
440 -     } else if size.Width != 0 && size.Height != 0 {
441 -         formats = []openrtb.Format{{
442 -             W: size.Width,
443 -             H: size.Height,
444 -         }}
445 -     }
446  
447       return append(formats, size.Multisize...)
448   }
    + 
    + func getNonZeroInt(overrideVal, originalVal int) int {
    +     if overrideVal > 0 {
    +         return overrideVal
    +     } else if originalVal > 0 {
    +         return originalVal
    +     }
    +     return -1
    + }
endpoints/openrtb2/amp_auction.go

[SyntaxNode]

Possibly. This is strictly a refactor PR. You're welcomed to explore that logic change in a separate PR.

[guscarreon]

Should we consider the overriden values in case the regular ones are 0?

378     if req.Imp[0].Banner != nil {
379         if format := makeFormatReplacement(ampParams.Size); len(format) != 0 {
380             req.Imp[0].Banner.Format = format
381         } else if ampParams.Size.Width != 0 {
382             setWidths(req.Imp[0].Banner.Format, ampParams.Size.Width)
383         } else if ampParams.Size.Height != 0 {
384             setHeights(req.Imp[0].Banner.Format, ampParams.Size.Height)
385 -       }
    +       } else if ampParams.Size.OverrideWidth != 0 {
    +           setWidths(req.Imp[0].Banner.Format, ampParams.Size.OverrideWidth)
    +       } else if ampParams.Size.OverrideHeight != 0 {
    +           setHeights(req.Imp[0].Banner.Format, ampParams.Size.OverrideHeight)
    +       }
386     }
endpoints/openrtb2/amp_auction.go

[SyntaxNode]

Possibly. With this PR I'm refactoring the parse flow, not the implementation.

[guscarreon]

Is there a possibility that req.TMax ends up being negative? Should we put a check?

[SyntaxNode]

I'm not sure. Could you please create an internal ticket for us to revisit?

[guscarreon]

Why do we need it to be a pointer? Not saying it is wrong, I just noticed it wasn't a pointer before.

[SyntaxNode]

I'm using a pointer to distinguish if a timeout value was provided and valid versus not provided and/or invalid. Previously, the parse method was in-line with its usage and we checked for a parse error immediately. Now there's the need to communicate a parse error differently. Go doesn't have many constructs for doing so, sadly.

[guscarreon]

Created internal ticket to list come of the remaining comments. LGTM