From 3e9556d9e75c9a8a0383e65406c6efc6c1cf0ad4 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 19 Mar 2025 18:08:33 -0400 Subject: [PATCH 1/9] Silence a warning from IntelliJ --- .../elasticsearch/ingest/geoip/MaxmindIpDataLookupsTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookupsTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookupsTests.java index 57ee2191a590d..88cff23cd9b7f 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookupsTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookupsTests.java @@ -155,7 +155,7 @@ public void testCountryWithMissingLocation() { ); } - public void testAsn() throws IOException { + public void testAsn() { assumeFalse("https://github.com/elastic/elasticsearch/issues/114266", Constants.WINDOWS); String databaseName = "GeoLite2-ASN.mmdb"; String ip = "82.171.64.0"; From 59b900bf0aea5f613b1cfc8a574092e0c72d88f9 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 19 Mar 2025 18:13:45 -0400 Subject: [PATCH 2/9] This variable cannot be null CityResponse and EnterpriseResponse use the null-object pattern here, so postal is never null. --- .../elasticsearch/ingest/geoip/MaxmindIpDataLookups.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java index 061da0c5d1036..193c12b2d2a0a 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java @@ -279,7 +279,7 @@ protected Map transform(final CityResponse response) { } } case POSTAL_CODE -> { - if (postal != null && postal.getCode() != null) { + if (postal.getCode() != null) { data.put("postal_code", postal.getCode()); } } @@ -552,14 +552,13 @@ protected Map transform(final EnterpriseResponse response) { } } case POSTAL_CODE -> { - if (postal != null && postal.getCode() != null) { + if (postal.getCode() != null) { data.put("postal_code", postal.getCode()); } } case POSTAL_CONFIDENCE -> { - Integer postalConfidence = postal.getConfidence(); - if (postalConfidence != null) { - data.put("postal_confidence", postalConfidence); + if (postal.getConfidence() != null) { + data.put("postal_confidence", postal.getConfidence()); } } case ASN -> { From ac58ba6e01cdd4f70a5436e18cb9e65fec6dc217 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Tue, 18 Mar 2025 11:20:26 -0400 Subject: [PATCH 3/9] Introduce another layer of indirection Allow a given lookup to create a cache-able record from the response from the database. At the moment every lookup just uses its response from the database as the cache-able record (this is fine). --- .../ingest/geoip/MaxmindIpDataLookups.java | 72 +++++++++++++++---- .../ingest/geoip/MaxMindSupportTests.java | 2 +- 2 files changed, 60 insertions(+), 14 deletions(-) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java index 193c12b2d2a0a..eaedeca0ae16a 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java @@ -107,7 +107,7 @@ static Function, IpDataLookup> getMaxmindLookup(final Dat }; } - static class AnonymousIp extends AbstractBase { + static class AnonymousIp extends AbstractBase { AnonymousIp(final Set properties) { super( properties, @@ -116,6 +116,11 @@ static class AnonymousIp extends AbstractBase { ); } + @Override + protected AnonymousIpResponse record(AnonymousIpResponse response) { + return response; + } + @Override protected Map transform(final AnonymousIpResponse response) { boolean isHostingProvider = response.isHostingProvider(); @@ -153,11 +158,16 @@ protected Map transform(final AnonymousIpResponse response) { } } - static class Asn extends AbstractBase { + static class Asn extends AbstractBase { Asn(Set properties) { super(properties, AsnResponse.class, (response, ipAddress, network, locales) -> new AsnResponse(response, ipAddress, network)); } + @Override + protected AsnResponse record(AsnResponse response) { + return response; + } + @Override protected Map transform(final AsnResponse response) { Long asn = response.getAutonomousSystemNumber(); @@ -189,11 +199,16 @@ protected Map transform(final AsnResponse response) { } } - static class City extends AbstractBase { + static class City extends AbstractBase { City(final Set properties) { super(properties, CityResponse.class, CityResponse::new); } + @Override + protected CityResponse record(CityResponse response) { + return response; + } + @Override protected Map transform(final CityResponse response) { com.maxmind.geoip2.record.Country country = response.getCountry(); @@ -305,7 +320,7 @@ protected Map transform(final CityResponse response) { } } - static class ConnectionType extends AbstractBase { + static class ConnectionType extends AbstractBase { ConnectionType(final Set properties) { super( properties, @@ -314,6 +329,11 @@ static class ConnectionType extends AbstractBase { ); } + @Override + protected ConnectionTypeResponse record(ConnectionTypeResponse response) { + return response; + } + @Override protected Map transform(final ConnectionTypeResponse response) { ConnectionTypeResponse.ConnectionType connectionType = response.getConnectionType(); @@ -333,11 +353,16 @@ protected Map transform(final ConnectionTypeResponse response) { } } - static class Country extends AbstractBase { + static class Country extends AbstractBase { Country(final Set properties) { super(properties, CountryResponse.class, CountryResponse::new); } + @Override + protected CountryResponse record(CountryResponse response) { + return response; + } + @Override protected Map transform(final CountryResponse response) { com.maxmind.geoip2.record.Country country = response.getCountry(); @@ -400,7 +425,7 @@ protected Map transform(final CountryResponse response) { } } - static class Domain extends AbstractBase { + static class Domain extends AbstractBase { Domain(final Set properties) { super( properties, @@ -409,6 +434,11 @@ static class Domain extends AbstractBase { ); } + @Override + protected DomainResponse record(DomainResponse response) { + return response; + } + @Override protected Map transform(final DomainResponse response) { String domain = response.getDomain(); @@ -428,11 +458,16 @@ protected Map transform(final DomainResponse response) { } } - static class Enterprise extends AbstractBase { + static class Enterprise extends AbstractBase { Enterprise(final Set properties) { super(properties, EnterpriseResponse.class, EnterpriseResponse::new); } + @Override + protected EnterpriseResponse record(EnterpriseResponse response) { + return response; + } + @Override protected Map transform(final EnterpriseResponse response) { com.maxmind.geoip2.record.Country country = response.getCountry(); @@ -651,11 +686,16 @@ protected Map transform(final EnterpriseResponse response) { } } - static class Isp extends AbstractBase { + static class Isp extends AbstractBase { Isp(final Set properties) { super(properties, IspResponse.class, (response, ipAddress, network, locales) -> new IspResponse(response, ipAddress, network)); } + @Override + protected IspResponse record(IspResponse response) { + return response; + } + @Override protected Map transform(final IspResponse response) { String isp = response.getIsp(); @@ -729,7 +769,7 @@ private interface ResponseBuilder { * * @param the intermediate type of {@link AbstractResponse} */ - private abstract static class AbstractBase implements IpDataLookup { + private abstract static class AbstractBase implements IpDataLookup { protected final Set properties; protected final Class clazz; @@ -748,24 +788,30 @@ public Set getProperties() { @Override public final Map getData(final IpDatabase ipDatabase, final String ipAddress) { - final RESPONSE response = ipDatabase.getResponse(ipAddress, this::lookup); + final RECORD response = ipDatabase.getResponse(ipAddress, this::lookup); return (response == null) ? Map.of() : transform(response); } @Nullable - private RESPONSE lookup(final Reader reader, final String ipAddress) throws IOException { + private RECORD lookup(final Reader reader, final String ipAddress) throws IOException { final InetAddress ip = InetAddresses.forString(ipAddress); final DatabaseRecord record = reader.getRecord(ip, clazz); final RESPONSE data = record.getData(); - return (data == null) ? null : builder.build(data, NetworkAddress.format(ip), record.getNetwork(), List.of("en")); + return (data == null) ? null : record(builder.build(data, NetworkAddress.format(ip), record.getNetwork(), List.of("en"))); } + /** + * Given a fully-built response object, create a record that is suitable for caching. If the fully-built response object + * itself is suitable for caching, then it is acceptable to simply return it. + */ + protected abstract RECORD record(RESPONSE response); + /** * Extract the configured properties from the retrieved response * @param response the non-null response that was retrieved * @return a mapping of properties for the ip from the response */ - protected abstract Map transform(RESPONSE response); + protected abstract Map transform(RECORD response); } @Nullable diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/MaxMindSupportTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/MaxMindSupportTests.java index 292a7e3c632d3..2bd928018588a 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/MaxMindSupportTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/MaxMindSupportTests.java @@ -645,7 +645,7 @@ private static Set> getUsedMaxMindResponseClas Method[] declaredMethods = declaredClass.getDeclaredMethods(); Optional nonAbstractTransformMethod = Arrays.stream(declaredMethods) .filter( - method -> method.getName().equals("transform") + method -> method.getName().equals("record") && method.getParameterTypes().length == 1 && Modifier.isAbstract(method.getParameterTypes()[0].getModifiers()) == false ) From 59cdf6d6fd2b992fab0b0b0563b12a3a0a7acc61 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Mon, 24 Mar 2025 10:20:46 -0400 Subject: [PATCH 4/9] Promote the Result helper record to IpDataLookup so that it can be reused. Of course that means that we have to tweak all the existing callers to use method access rather than field access, hence the bigger diff than one might otherwise imagine. --- .../ingest/geoip/IpDataLookup.java | 7 +++++ .../ingest/geoip/IpinfoIpDataLookups.java | 29 +++++++------------ 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java index 7442c8e930886..1f1e04274ac25 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java @@ -28,4 +28,11 @@ interface IpDataLookup { * @return the set of properties this lookup will provide */ Set getProperties(); + + /** + * A helper record that holds other records. Every ip data lookup will have an associated ip address that was looked up, as well + * as a network for which the record applies. Having a helper record prevents each individual response record from needing to + * track these bits of information. + */ + record Result(T result, String ip, String network) {} } diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java index 8ce2424844d9d..e6d609dd3b8d7 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java @@ -264,15 +264,15 @@ static class Asn extends AbstractBase { @Override protected Map transform(final Result result) { - AsnResult response = result.result; + AsnResult response = result.result(); Long asn = response.asn; String organizationName = response.name; - String network = result.network; + String network = result.network(); Map data = new HashMap<>(); for (Database.Property property : this.properties) { switch (property) { - case IP -> data.put("ip", result.ip); + case IP -> data.put("ip", result.ip()); case ASN -> { if (asn != null) { data.put("asn", asn); @@ -316,12 +316,12 @@ static class Country extends AbstractBase { @Override protected Map transform(final Result result) { - CountryResult response = result.result; + CountryResult response = result.result(); Map data = new HashMap<>(); for (Database.Property property : this.properties) { switch (property) { - case IP -> data.put("ip", result.ip); + case IP -> data.put("ip", result.ip()); case COUNTRY_ISO_CODE -> { String countryIsoCode = response.country; if (countryIsoCode != null) { @@ -359,12 +359,12 @@ static class Geolocation extends AbstractBase { @Override protected Map transform(final Result result) { - GeolocationResult response = result.result; + GeolocationResult response = result.result(); Map data = new HashMap<>(); for (Database.Property property : this.properties) { switch (property) { - case IP -> data.put("ip", result.ip); + case IP -> data.put("ip", result.ip()); case COUNTRY_ISO_CODE -> { String countryIsoCode = response.country; if (countryIsoCode != null) { @@ -418,12 +418,12 @@ static class PrivacyDetection extends AbstractBase { @Override protected Map transform(final Result result) { - PrivacyDetectionResult response = result.result; + PrivacyDetectionResult response = result.result(); Map data = new HashMap<>(); for (Database.Property property : this.properties) { switch (property) { - case IP -> data.put("ip", result.ip); + case IP -> data.put("ip", result.ip()); case HOSTING -> { if (response.hosting != null) { data.put("hosting", response.hosting); @@ -460,16 +460,9 @@ protected Map transform(final Result res } } - /** - * Just a little record holder -- there's the data that we receive via the binding to our record objects from the Reader via the - * getRecord call, but then we also need to capture the passed-in ip address that came from the caller as well as the network for - * the returned DatabaseRecord from the Reader. - */ - public record Result(T result, String ip, String network) {} - /** * The {@link IpinfoIpDataLookups.AbstractBase} is an abstract base implementation of {@link IpDataLookup} that - * provides common functionality for getting a {@link IpinfoIpDataLookups.Result} that wraps a record from a {@link IpDatabase}. + * provides common functionality for getting a {@link IpDataLookup.Result} that wraps a record from a {@link IpDatabase}. * * @param the record type that will be wrapped and returned */ @@ -491,7 +484,7 @@ public Set getProperties() { @Override public final Map getData(final IpDatabase ipDatabase, final String ipAddress) { final Result response = ipDatabase.getResponse(ipAddress, this::lookup); - return (response == null || response.result == null) ? Map.of() : transform(response); + return (response == null || response.result() == null) ? Map.of() : transform(response); } @Nullable From b6fc879f0b1bcc8f4dc65acb9b18b1e97bcbf64e Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Tue, 18 Mar 2025 11:53:28 -0400 Subject: [PATCH 5/9] Use a custom cache record for CountryResponse --- .../ingest/geoip/MaxmindIpDataLookups.java | 82 ++++++++++++------- 1 file changed, 52 insertions(+), 30 deletions(-) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java index eaedeca0ae16a..28c1adf924d81 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java @@ -25,12 +25,14 @@ import com.maxmind.geoip2.record.Location; import com.maxmind.geoip2.record.Postal; import com.maxmind.geoip2.record.Subdivision; +import com.maxmind.geoip2.record.Traits; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.elasticsearch.common.network.InetAddresses; import org.elasticsearch.common.network.NetworkAddress; import org.elasticsearch.core.Nullable; +import org.elasticsearch.ingest.geoip.IpDataLookup.Result; import java.io.IOException; import java.net.InetAddress; @@ -353,70 +355,90 @@ protected Map transform(final ConnectionTypeResponse response) { } } - static class Country extends AbstractBase { + record CacheableCountryResponse( + Boolean isInEuropeanUnion, + String countryIsoCode, + String countryName, + String continentCode, + String continentName, + Boolean registeredCountryIsInEuropeanUnion, + String registeredCountryIsoCode, + String registeredCountryName + ) {} + + static class Country extends AbstractBase> { Country(final Set properties) { super(properties, CountryResponse.class, CountryResponse::new); } @Override - protected CountryResponse record(CountryResponse response) { - return response; + protected Result record(CountryResponse response) { + final com.maxmind.geoip2.record.Country country = response.getCountry(); + final Continent continent = response.getContinent(); + final com.maxmind.geoip2.record.Country registeredCountry = response.getRegisteredCountry(); + final Traits traits = response.getTraits(); + return new Result<>( + new CacheableCountryResponse( + isInEuropeanUnion(country), + country.getIsoCode(), + country.getName(), + continent.getCode(), + continent.getName(), + isInEuropeanUnion(registeredCountry), + registeredCountry.getIsoCode(), + registeredCountry.getName() + ), + traits.getIpAddress(), + traits.getNetwork().toString() + ); } @Override - protected Map transform(final CountryResponse response) { - com.maxmind.geoip2.record.Country country = response.getCountry(); - com.maxmind.geoip2.record.Country registeredCountry = response.getRegisteredCountry(); - Continent continent = response.getContinent(); + protected Map transform(final Result result) { + CacheableCountryResponse response = result.result(); Map data = new HashMap<>(); for (Database.Property property : this.properties) { switch (property) { - case IP -> data.put("ip", response.getTraits().getIpAddress()); + case IP -> data.put("ip", result.ip()); case COUNTRY_IN_EUROPEAN_UNION -> { - Boolean isInEuropeanUnion = isInEuropeanUnion(country); - if (isInEuropeanUnion != null) { - data.put("country_in_european_union", isInEuropeanUnion); + if (response.isInEuropeanUnion != null) { + data.put("country_in_european_union", response.isInEuropeanUnion); } } case COUNTRY_ISO_CODE -> { - String countryIsoCode = country.getIsoCode(); - if (countryIsoCode != null) { - data.put("country_iso_code", countryIsoCode); + if (response.countryIsoCode != null) { + data.put("country_iso_code", response.countryIsoCode); } } case COUNTRY_NAME -> { - String countryName = country.getName(); - if (countryName != null) { - data.put("country_name", countryName); + if (response.countryName != null) { + data.put("country_name", response.countryName); } } case CONTINENT_CODE -> { - String continentCode = continent.getCode(); - if (continentCode != null) { - data.put("continent_code", continentCode); + if (response.continentCode != null) { + data.put("continent_code", response.continentCode); } } case CONTINENT_NAME -> { - String continentName = continent.getName(); - if (continentName != null) { - data.put("continent_name", continentName); + if (response.continentName != null) { + data.put("continent_name", response.continentName); } } case REGISTERED_COUNTRY_IN_EUROPEAN_UNION -> { - Boolean isInEuropeanUnion = isInEuropeanUnion(registeredCountry); - if (isInEuropeanUnion != null) { - data.put("registered_country_in_european_union", isInEuropeanUnion); + if (response.registeredCountryIsInEuropeanUnion != null) { + data.put("registered_country_in_european_union", response.registeredCountryIsInEuropeanUnion); } } case REGISTERED_COUNTRY_ISO_CODE -> { - if (registeredCountry.getIsoCode() != null) { - data.put("registered_country_iso_code", registeredCountry.getIsoCode()); + if (response.registeredCountryIsoCode != null) { + data.put("registered_country_iso_code", response.registeredCountryIsoCode); } } case REGISTERED_COUNTRY_NAME -> { - if (registeredCountry.getName() != null) { - data.put("registered_country_name", registeredCountry.getName()); + if (response.registeredCountryName != null) { + data.put("registered_country_name", response.registeredCountryName); } } } From a2ec9370f517276096c42ef27cda076c43a0315c Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 26 Mar 2025 10:22:38 -0400 Subject: [PATCH 6/9] Twiddle some docstrings --- .../elasticsearch/ingest/geoip/IpinfoIpDataLookups.java | 3 ++- .../elasticsearch/ingest/geoip/MaxmindIpDataLookups.java | 7 ++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java index e6d609dd3b8d7..1eeb08a22f90a 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java @@ -496,7 +496,8 @@ private Result lookup(final Reader reader, final String ipAddress) thr } /** - * Extract the configured properties from the retrieved response + * Extract the configured properties from the retrieved response. + * * @param response the non-null response that was retrieved * @return a mapping of properties for the ip from the response */ diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java index 28c1adf924d81..e9ae21de12c21 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java @@ -774,7 +774,7 @@ protected Map transform(final IspResponse response) { } /** - * As an internal detail, the {@code com.maxmind.geoip2.model } classes that are populated by + * As an internal detail, the {@code com.maxmind.geoip2.model} classes that are populated by * {@link Reader#getRecord(InetAddress, Class)} are kinda half-populated and need to go through a second round of construction * with context from the querying caller. This method gives us a place do that additional binding. Cleverly, the signature * here matches the constructor for many of these model classes exactly, so an appropriate implementation can 'just' be a method @@ -823,13 +823,14 @@ private RECORD lookup(final Reader reader, final String ipAddress) throws IOExce } /** - * Given a fully-built response object, create a record that is suitable for caching. If the fully-built response object + * Given a fully-populated response object, create a record that is suitable for caching. If the fully-populated response object * itself is suitable for caching, then it is acceptable to simply return it. */ protected abstract RECORD record(RESPONSE response); /** - * Extract the configured properties from the retrieved response + * Extract the configured properties from the retrieved response. + * * @param response the non-null response that was retrieved * @return a mapping of properties for the ip from the response */ From 791fb1d0c893dae0929d1f3704dfd2ec7b95054c Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 26 Mar 2025 10:22:53 -0400 Subject: [PATCH 7/9] Lampshade a small oddity --- .../org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java index e9ae21de12c21..a375bdb265300 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java @@ -795,6 +795,7 @@ private abstract static class AbstractBase properties; protected final Class clazz; + // see the docstring on ResponseBuilder to understand why this isn't yet another abstract method on this class protected final ResponseBuilder builder; AbstractBase(final Set properties, final Class clazz, final ResponseBuilder builder) { From 0428113a36ff5fd4eb5011a998d15fa007a5bf58 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 26 Mar 2025 11:16:17 -0400 Subject: [PATCH 8/9] Rename these variables There's too many nearby things named record or result or response. So here's a new word, 'entry'. A database has a bunch of entries in it. --- .../org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java | 6 +++--- .../elasticsearch/ingest/geoip/MaxmindIpDataLookups.java | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java index 1eeb08a22f90a..d548fa9bc98ea 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java @@ -490,9 +490,9 @@ public final Map getData(final IpDatabase ipDatabase, final Stri @Nullable private Result lookup(final Reader reader, final String ipAddress) throws IOException { final InetAddress ip = InetAddresses.forString(ipAddress); - final DatabaseRecord record = reader.getRecord(ip, clazz); - final RESPONSE data = record.getData(); - return (data == null) ? null : new Result<>(data, NetworkAddress.format(ip), record.getNetwork().toString()); + final DatabaseRecord entry = reader.getRecord(ip, clazz); + final RESPONSE data = entry.getData(); + return (data == null) ? null : new Result<>(data, NetworkAddress.format(ip), entry.getNetwork().toString()); } /** diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java index a375bdb265300..a2734c045236c 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java @@ -818,9 +818,9 @@ public final Map getData(final IpDatabase ipDatabase, final Stri @Nullable private RECORD lookup(final Reader reader, final String ipAddress) throws IOException { final InetAddress ip = InetAddresses.forString(ipAddress); - final DatabaseRecord record = reader.getRecord(ip, clazz); - final RESPONSE data = record.getData(); - return (data == null) ? null : record(builder.build(data, NetworkAddress.format(ip), record.getNetwork(), List.of("en"))); + final DatabaseRecord entry = reader.getRecord(ip, clazz); + final RESPONSE data = entry.getData(); + return (data == null) ? null : record(builder.build(data, NetworkAddress.format(ip), entry.getNetwork(), List.of("en"))); } /** From 9dd8964b66f8517920e10cbf6ddd8afa791910ff Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 26 Mar 2025 11:30:57 -0400 Subject: [PATCH 9/9] Rename this method --- .../ingest/geoip/MaxmindIpDataLookups.java | 22 ++++++++++--------- .../ingest/geoip/MaxMindSupportTests.java | 2 +- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java index a2734c045236c..fbc8f34ce6e21 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java @@ -119,7 +119,7 @@ static class AnonymousIp extends AbstractBase { } @Override - protected AsnResponse record(AsnResponse response) { + protected AsnResponse cacheableRecord(AsnResponse response) { return response; } @@ -207,7 +207,7 @@ static class City extends AbstractBase { } @Override - protected CityResponse record(CityResponse response) { + protected CityResponse cacheableRecord(CityResponse response) { return response; } @@ -332,7 +332,7 @@ static class ConnectionType extends AbstractBase record(CountryResponse response) { + protected Result cacheableRecord(CountryResponse response) { final com.maxmind.geoip2.record.Country country = response.getCountry(); final Continent continent = response.getContinent(); final com.maxmind.geoip2.record.Country registeredCountry = response.getRegisteredCountry(); @@ -457,7 +457,7 @@ static class Domain extends AbstractBase { } @Override - protected DomainResponse record(DomainResponse response) { + protected DomainResponse cacheableRecord(DomainResponse response) { return response; } @@ -486,7 +486,7 @@ static class Enterprise extends AbstractBase { } @Override - protected IspResponse record(IspResponse response) { + protected IspResponse cacheableRecord(IspResponse response) { return response; } @@ -820,14 +820,16 @@ private RECORD lookup(final Reader reader, final String ipAddress) throws IOExce final InetAddress ip = InetAddresses.forString(ipAddress); final DatabaseRecord entry = reader.getRecord(ip, clazz); final RESPONSE data = entry.getData(); - return (data == null) ? null : record(builder.build(data, NetworkAddress.format(ip), entry.getNetwork(), List.of("en"))); + return (data == null) + ? null + : cacheableRecord(builder.build(data, NetworkAddress.format(ip), entry.getNetwork(), List.of("en"))); } /** * Given a fully-populated response object, create a record that is suitable for caching. If the fully-populated response object * itself is suitable for caching, then it is acceptable to simply return it. */ - protected abstract RECORD record(RESPONSE response); + protected abstract RECORD cacheableRecord(RESPONSE response); /** * Extract the configured properties from the retrieved response. diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/MaxMindSupportTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/MaxMindSupportTests.java index 2bd928018588a..d769df0d932a2 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/MaxMindSupportTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/MaxMindSupportTests.java @@ -645,7 +645,7 @@ private static Set> getUsedMaxMindResponseClas Method[] declaredMethods = declaredClass.getDeclaredMethods(); Optional nonAbstractTransformMethod = Arrays.stream(declaredMethods) .filter( - method -> method.getName().equals("record") + method -> method.getName().equals("cacheableRecord") && method.getParameterTypes().length == 1 && Modifier.isAbstract(method.getParameterTypes()[0].getModifiers()) == false )