-
Notifications
You must be signed in to change notification settings - Fork 187
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
Greenbids fix geolookup: fetch from official MaxMind URL + mock dbReader UT #3626
Changes from all commits
0f41d3f
12e072b
4a8d2a3
2eb7a4c
00e7255
6217ac4
5e685d0
972cfd4
587d1ba
8e39877
b2f3c8a
e56b002
37c7be7
af3c123
98bf147
2cc7d99
62068e5
1055606
2d5ba29
b4ae0d7
3972151
2f991c3
a694373
2a7878b
760cbc6
0c372f9
3bbc02d
599d332
40113b4
cc4d6a6
8cdeae9
806e48c
9794643
a7cbc33
ea1b65c
829b83a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,13 +6,15 @@ | |
import com.fasterxml.jackson.databind.node.ObjectNode; | ||
import com.iab.openrtb.request.BidRequest; | ||
import com.iab.openrtb.request.Device; | ||
import com.iab.openrtb.request.Geo; | ||
import com.iab.openrtb.request.Imp; | ||
import com.maxmind.geoip2.DatabaseReader; | ||
import com.maxmind.geoip2.exception.GeoIp2Exception; | ||
import com.maxmind.geoip2.model.CountryResponse; | ||
import com.maxmind.geoip2.record.Country; | ||
import org.apache.commons.lang3.StringUtils; | ||
import org.prebid.server.exception.PreBidException; | ||
import org.prebid.server.geolocation.CountryCodeMapper; | ||
import org.prebid.server.hooks.modules.greenbids.real.time.data.config.DatabaseReaderFactory; | ||
import org.prebid.server.hooks.modules.greenbids.real.time.data.model.data.ThrottlingMessage; | ||
import org.prebid.server.proto.openrtb.ext.request.ExtImpPrebid; | ||
|
@@ -25,6 +27,7 @@ | |
import java.util.Collection; | ||
import java.util.Iterator; | ||
import java.util.List; | ||
import java.util.Locale; | ||
import java.util.Objects; | ||
import java.util.Optional; | ||
import java.util.stream.Collectors; | ||
|
@@ -35,9 +38,14 @@ public class GreenbidsInferenceDataService { | |
|
||
private final ObjectMapper mapper; | ||
|
||
public GreenbidsInferenceDataService(DatabaseReaderFactory dbReaderFactory, ObjectMapper mapper) { | ||
private final CountryCodeMapper countryCodeMapper; | ||
|
||
public GreenbidsInferenceDataService(DatabaseReaderFactory dbReaderFactory, | ||
ObjectMapper mapper, | ||
CountryCodeMapper countryCodeMapper) { | ||
this.databaseReaderFactory = Objects.requireNonNull(dbReaderFactory); | ||
this.mapper = Objects.requireNonNull(mapper); | ||
this.countryCodeMapper = Objects.requireNonNull(countryCodeMapper); | ||
} | ||
|
||
public List<ThrottlingMessage> extractThrottlingMessagesFromBidRequest(BidRequest bidRequest) { | ||
|
@@ -86,23 +94,38 @@ private List<ThrottlingMessage> extractMessagesForImp( | |
final String ip = Optional.ofNullable(bidRequest.getDevice()) | ||
.map(Device::getIp) | ||
.orElse(null); | ||
final String countryFromIp = getCountry(ip); | ||
final String country = Optional.ofNullable(bidRequest.getDevice()) | ||
.map(Device::getGeo) | ||
.map(Geo::getCountry) | ||
.map(countryCodeMapper::mapToAlpha2) | ||
.map(GreenbidsInferenceDataService::getCountryNameFromAlpha2) | ||
.filter(c -> !c.isEmpty()) | ||
.orElseGet(() -> getCountry(ip)); | ||
EvgeniiMunin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return createThrottlingMessages( | ||
bidderNode, | ||
impId, | ||
greenbidsUserAgent, | ||
countryFromIp, | ||
country, | ||
hostname, | ||
hourBucket, | ||
minuteQuadrant); | ||
} | ||
|
||
private String getCountry(String ip) { | ||
if (ip == null) { | ||
return null; | ||
} | ||
private static String getCountryNameFromAlpha2(String isoCode) { | ||
return StringUtils.isBlank(isoCode) | ||
? StringUtils.EMPTY | ||
: new Locale(StringUtils.EMPTY, isoCode).getDisplayCountry(); | ||
} | ||
|
||
private String getCountry(String ip) { | ||
final DatabaseReader databaseReader = databaseReaderFactory.getDatabaseReader(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NPE is possible There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added optional chain There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see the change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the nature of NPE here is the case when the geolookup database hasn't been loaded, so the underlying question here is whether it's fine and the PBS should be started up in that case at all, if yes then you'd better to handle this situation appropriately There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Modified to
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added optional chain + moved country extracting logic to a separate method
|
||
return ip != null && databaseReader != null | ||
? getCountryFromIpUsingDatabase(databaseReader, ip) | ||
: null; | ||
} | ||
|
||
private String getCountryFromIpUsingDatabase(DatabaseReader databaseReader, String ip) { | ||
try { | ||
final InetAddress inetAddress = InetAddress.getByName(ip); | ||
final CountryResponse response = databaseReader.country(inetAddress); | ||
|
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.
What about to use
RemoteFileSyncerV2
instead of this class? Just configureupdate-interval-ms: 0
. You can see example of using it withMaxMindGeoLocationService
.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.
As I remember from our discussion with @AntoxaAntoxic , we have discussed whether to reuse or not RemoteFileSyncerV2 and the public get() method of
RemoteFileSupplier
called in syncer and decided to not do that and to proceed with defining methodsvalidateResponse
,sendHttpRequest
,downloadFile
on the side of RTD module