Skip to content
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

Merged
merged 36 commits into from
Jan 17, 2025

Conversation

EvgeniiMunin
Copy link
Contributor

@EvgeniiMunin EvgeniiMunin commented Dec 13, 2024

🔧 Type of changes

  • new bid adapter
  • bid adapter update
  • new feature
  • new analytics adapter
  • new module
  • module update
  • bugfix
  • documentation
  • configuration
  • dependency update
  • tech debt (test coverage, refactorings, etc.)

✨ What's the context?

🧠 Rationale behind the change

  • We host the GeoLite2-Country.mmdb in Greenbids GCS bucket instead of open source URL: gs://greenbids-europe-west1-prebid-server-staging/GeoLite2-Country.mmdb
  • We mock the dbReader in unit test instead of loading real mmdb
  • We add the logic to fallback to getCountry(ip) only in case if Device::getGeo is null (geoLookupEnabled is false in GeoLocationServiceWrapper)

🔎 New Bid Adapter Checklist

  • verify email contact works
  • NO fully dynamic hostnames
  • geographic host parameters are NOT required
  • direct use of HTTP is prohibited - implement an existing Bidder interface that will do all the job
  • if the ORTB is just forwarded to the endpoint, use the generic adapter - define the new adapter as the alias of the generic adapter
  • cover an adapter configuration with an integration test

🧪 Test plan

How do you know the changes are safe to ship to production?

🏎 Quality check

  • Are your changes following our code style guidelines?
  • Are there any breaking changes in your code?
  • Does your test coverage exceed 90%?
  • Are there any erroneous console logs, debuggers or leftover code in your changes?

@EvgeniiMunin EvgeniiMunin changed the title Greenbids fix geolookup: mock dbReader TU + host GCS Greenbids fix geolookup: host and fetch GCS + mock dbReader TU Dec 13, 2024
@EvgeniiMunin EvgeniiMunin changed the title Greenbids fix geolookup: host and fetch GCS + mock dbReader TU Greenbids fix geolookup: host and fetch GCS + mock dbReader UT Dec 13, 2024
@EvgeniiMunin EvgeniiMunin force-pushed the greenbids-fix-geolookup branch from e5faad5 to ee640d3 Compare December 13, 2024 18:56
@EvgeniiMunin EvgeniiMunin marked this pull request as ready for review December 13, 2024 18:59
AntoxaAntoxic
AntoxaAntoxic previously approved these changes Dec 16, 2024
@EvgeniiMunin
Copy link
Contributor Author

Groovy IT is failing

Error:  Tests run: 38, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 21.95 s <<< FAILURE! -- in org.prebid.server.functional.tests.module.responsecorrenction.ResponseCorrectionSpec
Error:  org.prebid.server.functional.tests.module.responsecorrenction.ResponseCorrectionSpec.PBS should modify meta.mediaType and type for original response and also emit logs when response contains native meta.mediaType and adm without asset -- Time elapsed: 0.286 s <<< FAILURE!

I will push an empty commit to see if it is a flaky one

@EvgeniiMunin
Copy link
Contributor Author

Also need to not forget to remove .eventually(v -> tmpFile.close()) downloadFile and find workaround for ensuring file closing

As we discussed on Slack on the current state the usage of .eventually(v -> tmpFile.close()) leads to exception on file closing
java.lang.IllegalStateException: File handle is closed as it looks like pipeTo closes file automatically. Manual closing is recommended but leads to exception.

As the current objective of this PR is to fix the PBS codebase I propose to find the solution in the PR #3596

final Locale local = new Locale("", isoCode);
return local.getDisplayCountry();
}

private String getCountry(String ip) {
if (ip == null) {
return null;
}

final DatabaseReader databaseReader = databaseReaderFactory.getDatabaseReader();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NPE is possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added optional chain

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the change

Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified to

private static String getCountryNameFromAlpha2(String isoCode) {
        return Optional.ofNullable(isoCode)
                .filter(code -> !code.isBlank())
                .map(code -> new Locale("", code))
                .map(Locale::getDisplayCountry)
                .orElse("");
    }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

databaseReaderFactory.getDatabaseReader() returns null if the file has been loaded

Copy link
Contributor Author

@EvgeniiMunin EvgeniiMunin Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added optional chain + moved country extracting logic to a separate method

private String getCountry(String ip) {
        if (ip == null) {
            return null;
        }

        return Optional.ofNullable(databaseReaderFactory.getDatabaseReader())
                .map(dbReader -> getCountryFromIpUsingDatabase(dbReader, ip))
                .orElse(null);
    }

    private String getCountryFromIpUsingDatabase(DatabaseReader databaseReader, String ip) {
        try {
            final InetAddress inetAddress = InetAddress.getByName(ip);
            final CountryResponse response = databaseReader.country(inetAddress);
            final Country country = response.getCountry();
            return country.getName();
        } catch (IOException | GeoIp2Exception e) {
            throw new PreBidException("Failed to fetch country from geoLite DB", e);
        }
    }

@EvgeniiMunin EvgeniiMunin changed the title Greenbids fix geolookup: host and fetch GCS + mock dbReader UT Greenbids fix geolookup: host and fetch from official MaxMind URL + mock dbReader UT Jan 6, 2025
@EvgeniiMunin EvgeniiMunin changed the title Greenbids fix geolookup: host and fetch from official MaxMind URL + mock dbReader UT Greenbids fix geolookup: fetch from official MaxMind URL + mock dbReader UT Jan 6, 2025
@AntoxaAntoxic AntoxaAntoxic removed the do not merge Not the time for merging yet label Jan 7, 2025
@EvgeniiMunin EvgeniiMunin force-pushed the greenbids-fix-geolookup branch from 6bea4a6 to cc4d6a6 Compare January 7, 2025 15:45
AntoxaAntoxic
AntoxaAntoxic previously approved these changes Jan 8, 2025
Copy link
Collaborator

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 configure update-interval-ms: 0. You can see example of using it with MaxMindGeoLocationService.

Copy link
Contributor Author

@EvgeniiMunin EvgeniiMunin Jan 15, 2025

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 methods validateResponse, sendHttpRequest, downloadFile on the side of RTD module

image

private Future<Void> downloadFile(String downloadUrl, String tmpPath) {
return fileSystem.open(tmpPath, new OpenOptions())
.compose(tmpFile -> sendHttpRequest(downloadUrl)
.compose(response -> response.pipeTo(tmpFile))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

                        .compose(response -> response.pipeTo(tmpFile))
                        .onComplete(result -> tmpFile.close()))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding tmpFile.close() manual tmpFile closing results in java.lang.IllegalStateException: File handle is closed. The reason is that after invoking response.pipeTo(tmpFile) it closes tmpFile automatically by default and then  .onComplete(result -> mpFile.close()) trying double close it and fails. We previously discussed with @AntoxaAntoxic and tried to replace onComplete with .eventually(v -> tmpFile.close()) but result was the same.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EvgeniiMunin Seems you are right. Then please, change it a bit in your code and in RemoteFileSupplier if you don't mind:

                .compose(tmpFile -> sendHttpRequest(...)
                        .onFailure(ignore -> tmpFile.close())
                        .compose(response -> response.pipeTo(tmpFile)))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

CTMBNara
CTMBNara previously approved these changes Jan 16, 2025
@CTMBNara CTMBNara merged commit e9417e3 into prebid:master Jan 17, 2025
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants