Skip to content

Commit dea9dae

Browse files
0c0w30c0w3
committed
Bug 1971640 - fetch_geoname_alternates is broken when two different admin divisions at the same level have the same code
1 parent dc7b9c1 commit dea9dae

File tree

1 file changed

+138
-10
lines changed

1 file changed

+138
-10
lines changed

‎components/suggest/src/geoname.rs‎

Lines changed: 138 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -493,10 +493,15 @@ impl SuggestDao<'_> {
493493
AND g.country_code = :country
494494
)
495495
-- The row matches one of the geoname's admin divisions
496-
OR (g.feature_code = 'ADM1' AND g.admin1_code = :admin1)
497-
OR (g.feature_code = 'ADM2' AND g.admin2_code = :admin2)
498-
OR (g.feature_code = 'ADM3' AND g.admin3_code = :admin3)
499-
OR (g.feature_code = 'ADM4' AND g.admin4_code = :admin4)
496+
OR (
497+
g.country_code = :country
498+
AND (
499+
(g.feature_code = 'ADM1' AND g.admin1_code = :admin1)
500+
OR (g.feature_code = 'ADM2' AND g.admin2_code = :admin2)
501+
OR (g.feature_code = 'ADM3' AND g.admin3_code = :admin3)
502+
OR (g.feature_code = 'ADM4' AND g.admin4_code = :admin4)
503+
)
504+
)
500505
)
501506
ORDER BY
502507
-- Group rows for the same geoname together
@@ -1264,6 +1269,19 @@ pub(crate) mod tests {
12641269
"longitude": "-2.97794",
12651270
},
12661271

1272+
// Germany
1273+
{
1274+
"id": 2921044,
1275+
"name": "Federal Republic of Germany",
1276+
"feature_class": "A",
1277+
"feature_code": "PCLI",
1278+
"country": "DE",
1279+
"admin1": "00",
1280+
"population": 82927922,
1281+
"latitude": "51.5",
1282+
"longitude": "10.5",
1283+
},
1284+
12671285
// Gößnitz, DE (has non-basic-Latin chars and an `ascii_name`)
12681286
{
12691287
"id": 2918770,
@@ -1280,6 +1298,36 @@ pub(crate) mod tests {
12801298
"latitude": "50.88902",
12811299
"longitude": "12.43292",
12821300
},
1301+
1302+
// Rheinland-Pfalz (similar to ON, Canada: both are admin1's and
1303+
// both have the same admin1 code)
1304+
{
1305+
"id": 2847618,
1306+
"name": "Rheinland-Pfalz",
1307+
"feature_class": "A",
1308+
"feature_code": "ADM1",
1309+
"country": "DE",
1310+
"admin1": "08",
1311+
"population": 4093903,
1312+
"latitude": "49.66667",
1313+
"longitude": "7.5",
1314+
},
1315+
1316+
// Mainz, DE (city in Rheinland-Pfalz)
1317+
{
1318+
"id": 2874225,
1319+
"name": "Mainz",
1320+
"feature_class": "P",
1321+
"feature_code": "PPLA",
1322+
"country": "DE",
1323+
"admin1": "08",
1324+
"admin2": "00",
1325+
"admin3": "07315",
1326+
"admin4": "07315000",
1327+
"population": 217123,
1328+
"latitude": "49.98419",
1329+
"longitude": "8.2791",
1330+
},
12831331
])
12841332
}
12851333

@@ -1770,6 +1818,21 @@ pub(crate) mod tests {
17701818
}
17711819
}
17721820

1821+
pub(crate) fn germany() -> Geoname {
1822+
Geoname {
1823+
geoname_id: 2921044,
1824+
geoname_type: GeonameType::Country,
1825+
name: "Federal Republic of Germany".to_string(),
1826+
feature_class: "A".to_string(),
1827+
feature_code: "PCLI".to_string(),
1828+
country_code: "DE".to_string(),
1829+
admin_division_codes: [(1, "00".to_string())].into(),
1830+
population: 82927922,
1831+
latitude: "51.5".to_string(),
1832+
longitude: "10.5".to_string(),
1833+
}
1834+
}
1835+
17731836
pub(crate) fn goessnitz() -> Geoname {
17741837
Geoname {
17751838
geoname_id: 2918770,
@@ -1791,6 +1854,42 @@ pub(crate) mod tests {
17911854
}
17921855
}
17931856

1857+
pub(crate) fn rheinland_pfalz() -> Geoname {
1858+
Geoname {
1859+
geoname_id: 2847618,
1860+
geoname_type: GeonameType::AdminDivision { level: 1 },
1861+
name: "Rheinland-Pfalz".to_string(),
1862+
feature_class: "A".to_string(),
1863+
feature_code: "ADM1".to_string(),
1864+
country_code: "DE".to_string(),
1865+
admin_division_codes: [(1, "08".to_string())].into(),
1866+
population: 4093903,
1867+
latitude: "49.66667".to_string(),
1868+
longitude: "7.5".to_string(),
1869+
}
1870+
}
1871+
1872+
pub(crate) fn mainz() -> Geoname {
1873+
Geoname {
1874+
geoname_id: 2874225,
1875+
geoname_type: GeonameType::City,
1876+
name: "Mainz".to_string(),
1877+
feature_class: "P".to_string(),
1878+
feature_code: "PPLA".to_string(),
1879+
country_code: "DE".to_string(),
1880+
admin_division_codes: [
1881+
(1, "08".to_string()),
1882+
(2, "00".to_string()),
1883+
(3, "07315".to_string()),
1884+
(4, "07315000".to_string()),
1885+
]
1886+
.into(),
1887+
population: 217123,
1888+
latitude: "49.98419".to_string(),
1889+
longitude: "8.2791".to_string(),
1890+
}
1891+
}
1892+
17941893
#[test]
17951894
fn is_related_to() -> anyhow::Result<()> {
17961895
// The geonames in each vec should be pairwise related.
@@ -1799,6 +1898,7 @@ pub(crate) mod tests {
17991898
vec![waterloo_al(), al(), us()],
18001899
vec![waterloo_on(), on(), canada()],
18011900
vec![liverpool_city(), liverpool_metro(), england(), uk()],
1901+
vec![mainz(), rheinland_pfalz(), germany()],
18021902
];
18031903
for geonames in tests {
18041904
for g in &geonames {
@@ -1844,6 +1944,13 @@ pub(crate) mod tests {
18441944
vec![england(), us(), canada()],
18451945
vec![al(), ia(), on(), england()],
18461946
vec![us(), canada(), uk()],
1947+
// ON, Canada and Rheinland-Pfalz are both admin1's and both have
1948+
// the same admin1 code, but they're not related
1949+
vec![on(), rheinland_pfalz()],
1950+
// Mainz is a city in Rheinland-Pfalz
1951+
vec![on(), mainz()],
1952+
// Waterloo, ON is a city in ON
1953+
vec![rheinland_pfalz(), waterloo_on()],
18471954
];
18481955
for geonames in tests {
18491956
for a_and_b in geonames.iter().permutations(2) {
@@ -1936,14 +2043,14 @@ pub(crate) mod tests {
19362043
abbreviation: Some("NY".to_string()),
19372044
},
19382045
country: Some(AlternateNames {
1939-
primary: "United States".to_string(),
2046+
primary: us().name,
19402047
localized: Some("United States".to_string()),
19412048
abbreviation: Some("US".to_string()),
19422049
}),
19432050
admin_divisions: [(
19442051
1,
19452052
AlternateNames {
1946-
primary: ny_state().name.clone(),
2053+
primary: ny_state().name,
19472054
localized: Some("New York".to_string()),
19482055
abbreviation: Some("NY".to_string()),
19492056
},
@@ -1966,7 +2073,7 @@ pub(crate) mod tests {
19662073
admin_divisions: [(
19672074
1,
19682075
AlternateNames {
1969-
primary: on().name.clone(),
2076+
primary: on().name,
19702077
localized: Some("Ontario".to_string()),
19712078
abbreviation: Some("ON".to_string()),
19722079
},
@@ -1980,30 +2087,51 @@ pub(crate) mod tests {
19802087
abbreviation: None,
19812088
},
19822089
country: Some(AlternateNames {
1983-
primary: "United Kingdom of Great Britain and Northern Ireland".to_string(),
2090+
primary: uk().name,
19842091
localized: Some("United Kingdom".to_string()),
19852092
abbreviation: Some("UK".to_string()),
19862093
}),
19872094
admin_divisions: [
19882095
(
19892096
1,
19902097
AlternateNames {
1991-
primary: england().name.clone(),
2098+
primary: england().name,
19922099
localized: Some("England".to_string()),
19932100
abbreviation: None,
19942101
},
19952102
),
19962103
(
19972104
2,
19982105
AlternateNames {
1999-
primary: liverpool_metro().name.clone(),
2106+
primary: liverpool_metro().name,
20002107
localized: Some("Liverpool".to_string()),
20012108
abbreviation: Some("LIV".to_string()),
20022109
},
20032110
),
20042111
]
20052112
.into(),
20062113
}),
2114+
Test::new(mainz(), |g| GeonameAlternates {
2115+
geoname: AlternateNames {
2116+
primary: g.name.clone(),
2117+
localized: Some(g.name.clone()),
2118+
abbreviation: None,
2119+
},
2120+
country: Some(AlternateNames {
2121+
primary: germany().name,
2122+
localized: Some(germany().name),
2123+
abbreviation: None,
2124+
}),
2125+
admin_divisions: [(
2126+
1,
2127+
AlternateNames {
2128+
primary: rheinland_pfalz().name,
2129+
localized: Some(rheinland_pfalz().name),
2130+
abbreviation: None,
2131+
},
2132+
)]
2133+
.into(),
2134+
}),
20072135
Test::new(punctuation_city(0), |g| GeonameAlternates {
20082136
geoname: AlternateNames {
20092137
primary: g.name.clone(),

0 commit comments

Comments
 (0)