Copilot commented on code in PR #12948:
URL: https://github.com/apache/trafficserver/pull/12948#discussion_r2898382465
##########
plugins/header_rewrite/conditions_geo_maxmind.cc:
##########
@@ -91,33 +92,36 @@ MMConditionGeo::get_geo_string(const sockaddr *addr) const
return ret;
}
- const char *field_name;
+ MMDB_entry_data_s entry_data;
+
switch (_geo_qual) {
case GEO_QUAL_COUNTRY:
- field_name = "country_code";
+ status = MMDB_get_value(&result.entry, &entry_data, "country", "names",
"en", NULL);
Review Comment:
The `GEO_QUAL_COUNTRY` lookup was changed to `"country", "names", "en"`,
which returns the *English country name* (e.g. "South Korea"). Repo
documentation and the GeoIP backend currently define `%{GEO:COUNTRY}` as the
*country code* (e.g. "US"), so this is a cross-backend behavior change. If the
intent is to keep `%{GEO:COUNTRY}` consistent, use the `"country", "iso_code"`
path instead (and consider adding a separate qualifier later for the localized
name), or update the docs/other backend accordingly.
```suggestion
status = MMDB_get_value(&result.entry, &entry_data, "country",
"iso_code", NULL);
```
##########
plugins/header_rewrite/conditions_geo_maxmind.cc:
##########
@@ -91,33 +92,36 @@ MMConditionGeo::get_geo_string(const sockaddr *addr) const
return ret;
}
- const char *field_name;
+ MMDB_entry_data_s entry_data;
+
switch (_geo_qual) {
case GEO_QUAL_COUNTRY:
- field_name = "country_code";
+ status = MMDB_get_value(&result.entry, &entry_data, "country", "names",
"en", NULL);
+ break;
+ case GEO_QUAL_COUNTRY_ISO:
+ status = MMDB_get_value(&result.entry, &entry_data, "country", "iso_code",
NULL);
break;
case GEO_QUAL_ASN_NAME:
- field_name = "autonomous_system_organization";
+ status = MMDB_get_value(&result.entry, &entry_data,
"autonomous_system_organization", NULL);
break;
default:
Dbg(pi_dbg_ctl, "Unsupported field %d", _geo_qual);
+ MMDB_free_entry_data_list(entry_data_list);
return ret;
- break;
}
- MMDB_entry_data_s entry_data;
-
- status = MMDB_get_value(&result.entry, &entry_data, field_name, NULL);
if (MMDB_SUCCESS != status) {
- Dbg(pi_dbg_ctl, "ERROR on get value asn value: %s", MMDB_strerror(status));
+ Dbg(pi_dbg_ctl, "Error looking up geo string field: %s",
MMDB_strerror(status));
+ MMDB_free_entry_data_list(entry_data_list);
return ret;
}
- ret = std::string(entry_data.utf8_string, entry_data.data_size);
- if (nullptr != entry_data_list) {
- MMDB_free_entry_data_list(entry_data_list);
+ if (entry_data.has_data && entry_data.type == MMDB_DATA_TYPE_UTF8_STRING) {
+ ret = std::string(entry_data.utf8_string, entry_data.data_size);
}
+ MMDB_free_entry_data_list(entry_data_list);
+
return ret;
Review Comment:
`entry_data_list` is now only used for the `nullptr` check and to be freed
on return paths, but the actual field retrieval uses `MMDB_get_value()` (which
doesn’t require a data list). This means every geo lookup still pays the cost
of `MMDB_get_entry_data_list()` allocation/traversal even though the list isn’t
consumed. Consider removing the `MMDB_get_entry_data_list()`/`entry_data_list`
block entirely and rely on `result.found_entry` + `MMDB_get_value()` +
`entry_data.has_data` checks for a cheaper lookup path.
##########
plugins/header_rewrite/conditions_geo_maxmind.cc:
##########
@@ -91,33 +92,36 @@ MMConditionGeo::get_geo_string(const sockaddr *addr) const
return ret;
}
- const char *field_name;
+ MMDB_entry_data_s entry_data;
+
switch (_geo_qual) {
case GEO_QUAL_COUNTRY:
- field_name = "country_code";
+ status = MMDB_get_value(&result.entry, &entry_data, "country", "names",
"en", NULL);
+ break;
+ case GEO_QUAL_COUNTRY_ISO:
+ status = MMDB_get_value(&result.entry, &entry_data, "country", "iso_code",
NULL);
break;
Review Comment:
`GEO_QUAL_COUNTRY_ISO` is handled here as a string (via `MMDB_get_value(...,
"country", "iso_code", ...)`), but `ConditionGeo::set_qualifier()` currently
marks `COUNTRY-ISO` as an int-type qualifier, which causes header expansion /
condition evaluation to call `get_geo_int()` instead. With the current design,
`%{GEO:COUNTRY-ISO}` will still end up as `-1` on the MaxMind backend because
`get_geo_int()` doesn’t implement `GEO_QUAL_COUNTRY_ISO`. Align the qualifier’s
type/semantics with the implementation (either make `COUNTRY-ISO` a string
qualifier for all GEO backends, or keep it int-type and implement it in
`get_geo_int()` for MaxMind).
```suggestion
```
##########
plugins/header_rewrite/conditions_geo_maxmind.cc:
##########
@@ -91,33 +92,36 @@ MMConditionGeo::get_geo_string(const sockaddr *addr) const
return ret;
}
- const char *field_name;
+ MMDB_entry_data_s entry_data;
+
switch (_geo_qual) {
case GEO_QUAL_COUNTRY:
- field_name = "country_code";
+ status = MMDB_get_value(&result.entry, &entry_data, "country", "names",
"en", NULL);
+ break;
+ case GEO_QUAL_COUNTRY_ISO:
+ status = MMDB_get_value(&result.entry, &entry_data, "country", "iso_code",
NULL);
break;
case GEO_QUAL_ASN_NAME:
- field_name = "autonomous_system_organization";
+ status = MMDB_get_value(&result.entry, &entry_data,
"autonomous_system_organization", NULL);
break;
Review Comment:
This change updates the MMDB field paths and adds new qualifier handling,
but there don’t appear to be existing gold tests covering `%{GEO:...}` behavior
for the MaxMind backend. Adding an automated test that runs header_rewrite with
a small test MMDB fixture (or a generated minimal MMDB) would help prevent
regressions in the lookup paths and qualifier/type handling.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
