Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Places / refactoring get pois #15

Merged
merged 6 commits into from Mar 27, 2013

Conversation

Projects
None yet
2 participants
Contributor

martinfilliau commented Feb 28, 2013

Refactored service to allow the get/search of more than one ID at a time.
The goal is to do only one query per POI to get parent/children in the HAL representation, and consequently dramatically reduce the number of (HTTP) requests done to Solr.

Contributor

martinfilliau commented Feb 28, 2013

NB: this impacts other moxie projects as the API has changed (services).

@davbo davbo commented on an outdated diff Mar 26, 2013

moxie/places/services.py
- def search_place_by_identifier(self, ident):
+ def search_places_by_identifiers(self, idents):
@davbo

davbo Mar 26, 2013

Contributor

Perhaps if we maintain the old method signature and add a further API to get a group of identifiers? This way we don't break backward compatibility. Should be quite a simple function just return self.search_places_by_identifiers([ident]) (I guess)

@davbo davbo commented on the diff Mar 26, 2013

moxie/tests/test_places_services.py
@@ -10,6 +10,6 @@ def test_get_results_by_id(self):
with mock.patch('moxie.places.services.doc_to_poi') as mock_doc_to_poi:
poi_service = POIService()
mock_doc_to_poi.return_value = None
- results = poi_service.get_place_by_identifier('123')
@davbo

davbo Mar 26, 2013

Contributor

Above comment will mean reverting this change. We should probably have a test for returning multiple POI's also?

@davbo davbo commented on the diff Mar 26, 2013

moxie/places/representations.py
- representation.update_link('child', url_for(self.endpoint, ident=child),
- title=p.name, type=p.type, type_name=p.type_name)
- else:
- representation.update_link('child', url_for(self.endpoint, ident=child))
+ if poi_service and self.add_parent_children_links:
+ # Merging all IDs (parent and children) into one set to
+ # do only one query to the service
+ pois_ids = set(self.poi.children)
+ if self.poi.parent:
+ pois_ids.add(self.poi.parent)
+ if pois_ids:
+ pois_objects = poi_service.get_places_by_identifiers(pois_ids)
+ # ease lookup by having a dict with ID as key
+ pois = dict((poi.id, poi) for poi in pois_objects)
+
+ def add_link(relation, identifier):
@davbo

davbo Mar 26, 2013

Contributor

Shouldn't this be a method on on the HALPOIRepresentation? Can't see why it's a closure...

davbo added a commit that referenced this pull request Mar 27, 2013

@davbo davbo merged commit e0a3f1c into master Mar 27, 2013

1 check failed

default The Travis build failed
Details

@davbo davbo deleted the refactor_get_pois branch Mar 27, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment