Kobler: New adapter (#3667) #3684
Conversation
| try { | ||
| final ExtImpKobler impExt = parseImpExt(imp); | ||
|
|
||
| if (bidRequest.getImp().indexOf(imp) == 0) { |
There was a problem hiding this comment.
don't use indexOf, it spends unnecessary execution time to check each imp
| } | ||
|
|
||
| final Imp modifiedImp = modifyImp(imp, impExt, bidRequest); | ||
| requests.add(makeHttpRequest(bidRequest, modifiedImp, testMode)); |
There was a problem hiding this comment.
the bidder should send only a single request with all the imps instead of a request per imp
| return imp.toBuilder() | ||
| .bidfloor(resolvedBidFloor.getValue()) | ||
| .bidfloorcur(resolvedBidFloor.getCurrency()) | ||
| .ext(mapper.mapper().valueToTree(extImpKobler)) |
There was a problem hiding this comment.
I don't see imp.ext change in Go
|
|
||
| for (Imp imp : bidRequest.getImp()) { | ||
| try { | ||
| final ExtImpKobler impExt = parseImpExt(imp); |
There was a problem hiding this comment.
we don't need to parse imp.ext if it's not the first imp
| return bidsFromResponse(bidResponse, errors); | ||
| } | ||
|
|
||
| private List<BidderBid> bidsFromResponse(BidResponse bidResponse, List<BidderError> errors) { |
There was a problem hiding this comment.
the errors list is redundant
| private BidType getBidType(Bid bid) { | ||
| final Integer markupType = ObjectUtils.defaultIfNull(bid.getMtype(), 0); | ||
|
|
||
| return switch (markupType) { | ||
| case 1 -> BidType.banner; | ||
| default -> throw new PreBidException( | ||
| "could not define media type for impression: " + bid.getImpid()); | ||
| }; | ||
| } |
There was a problem hiding this comment.
that's just not how it works in Go
| @@ -0,0 +1,12 @@ | |||
| adapters: | |||
| kobler: | |||
| endpoint: "https://bid.essrtb.com/bid/prebid_server_rtb_call" | |||
There was a problem hiding this comment.
the endpoint-compression is missed
| BidderDeps koblerBidderDeps(BidderConfigurationProperties koblerConfigurationProperties, | ||
| CurrencyConversionService currencyConversionService, | ||
| @NotBlank @Value("${external-url}") String externalUrl, | ||
| JacksonMapper mapper) { |
There was a problem hiding this comment.
I used a checkstyle and it didnt show mistakes. How it should looks like?
| final ExtImpKobler impExt = parseImpExt(imp); | ||
|
|
||
| if (bidRequest.getImp().indexOf(imp) == 0) { | ||
| testMode = impExt.getTest(); |
There was a problem hiding this comment.
NPE will be thrown if imp.ext.test is NULL
| boolean testMode = false; | ||
| final List<Imp> modifiedImps = new ArrayList<>(); | ||
|
|
||
| final List<String> currencies = bidRequest.getCur() != null |
There was a problem hiding this comment.
bidRequest.getCur() can't be null
| return Result.withErrors(errors); | ||
| } | ||
|
|
||
| modifiedRequest = modifiedRequest.toBuilder().imp(modifiedImps).build(); |
There was a problem hiding this comment.
please modify the request once
| private Imp processImp(BidRequest bidRequest, Imp imp, List<BidderError> errors) { | ||
| if (imp.getBidfloor() != null | ||
| && imp.getBidfloor().compareTo(BigDecimal.ZERO) > 0 | ||
| && imp.getBidfloorcur() != null) { | ||
| final String bidFloorCur = imp.getBidfloorcur().toUpperCase(); | ||
| if (!DEFAULT_BID_CURRENCY.equals(bidFloorCur)) { | ||
| try { | ||
| final BigDecimal convertedPrice = currencyConversionService.convertCurrency( | ||
| imp.getBidfloor(), | ||
| bidRequest, | ||
| bidFloorCur, | ||
| DEFAULT_BID_CURRENCY | ||
| ); | ||
| return imp.toBuilder() | ||
| .bidfloor(convertedPrice) | ||
| .bidfloorcur(DEFAULT_BID_CURRENCY) | ||
| .build(); | ||
| } catch (PreBidException e) { | ||
| errors.add(BidderError.badInput(e.getMessage())); | ||
| } | ||
| } | ||
| } | ||
| return imp; | ||
| } |
There was a problem hiding this comment.
why did you change the whole code here? the last time it was better
| return imp; | ||
| } | ||
|
|
||
| public boolean extractTestMode(Imp imp) { |
| if (modifiedImps.size() == 1) { | ||
| testMode = extractTestMode(processedImp); | ||
| } |
There was a problem hiding this comment.
the Go version tries to retrieve the test field only from the first imp, in your implementation it's taken from the first VALID imp
| try { | ||
| final ExtPrebid<?, ExtImpKobler> extPrebid = mapper.mapper().convertValue(imp.getExt(), | ||
| KOBLER_EXT_TYPE_REFERENCE); | ||
| final ExtImpKobler extImpKobler = extPrebid != null ? extPrebid.getBidder() : null; |
There was a problem hiding this comment.
parse the imp.ext in the same way you did before, please make it in the same style other bidders do
| try { | ||
| return Result.of(Collections.singletonList( | ||
| HttpRequest.<BidRequest>builder() | ||
| .method(HttpMethod.POST) | ||
| .uri(endpoint) | ||
| .headers(HttpUtil.headers()) | ||
| .body(mapper.encodeToBytes(modifiedRequest)) | ||
| .payload(modifiedRequest) | ||
| .build() | ||
| ), errors); | ||
| } catch (EncodeException e) { | ||
| errors.add(BidderError.badInput("Failed to encode request: " + e.getMessage())); | ||
| return Result.withErrors(errors); | ||
| } |
There was a problem hiding this comment.
use BidderUtil.defaultRequest instead
🔧 Type of changes
✨ What's the context?
#3667