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

Fix for GeoJSON output erroneously producing an Array for the postalcode field. #1684

Merged
merged 3 commits into from
Jan 23, 2025

Conversation

missinglink
Copy link
Member

@missinglink missinglink commented Jan 23, 2025

This PR fixes an issue with the GeoJSON output which is erroneously producing an Array for the postalcode field for some records (ie. only those with aliases)

This was a tricky bug to track down, it's a combination of this cast which has been around for 9 years combined with some more modern work which triggered it.

We have some logic to 'flatten' array values to strings in the API codebase (helper/fieldValue.js), but what's happening here is that the Array coming back from elasticsearch is being wrapped in a second array, getStringValue() only flattens a single level.


The rest of this PR is me adding tests to try and understand what middleware/renamePlacenames did, and then renaming it to make it clearer what exactly it does 🤷


Screenshot 2025-01-23 at 15 14 19

https://pelias.github.io/compare/#/v1/search?text=kinkerstraat+175F

@missinglink
Copy link
Member Author

missinglink commented Jan 23, 2025

Worth noting an oddity I found...

The postalcode property can be set via either address_parts.zip or a parent.postalcode.

We correctly favour address_parts.zip where available.

However we end up merging in the postalcode_a, postalcode_gid & postalcode_src fields from the parent when available too.

So what we end up is a sort of mishmash of the two:

      postalcode_a: 'parent postalcode_a value',
      postalcode_gid: 'parent postalcode_id value',
      postalcode_source: 'parent postalcode_source value',

      postalcode: 'zip value',

Comment on lines 13 to 18
function setup() {
return renamePlacenames;
return mapFields;
}

function renamePlacenames(req, res, next) {
function mapFields(req, res, next) {
// do nothing if no result data set
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, much better naming.

Copy link
Member Author

Choose a reason for hiding this comment

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

I forgot to update all the references 😢
Just pushed rebase to resolve.

Copy link
Member

Choose a reason for hiding this comment

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

oof. And no tests caught it !? I thought we'd have so many tests that can't happen

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, because it was in routes which isn't tested.

@missinglink missinglink merged commit 8f14bcf into master Jan 23, 2025
6 checks passed
@missinglink missinglink deleted the middleware-refactor branch January 23, 2025 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants