Town6: Add API endpoints for forms, resources, people, meetings, political businesses and parliament#2347
Conversation
…, political businesses and parliament
Daverball
left a comment
Daverball
left a comment
There was a problem hiding this comment.
Looks like a good start. I'm not happy with the handling of ExternalLink however, they should be properly mixed into the pagination, so we don't lie about our items per page and/or how many pages there are.
There is a more general problem with models that make use of AccessExtension and UTCPublicationMixin (for some models you will only find those on the onegov.org specific subclass, but you still need to handle them in that case), since some of these collections can contain non-public entries and there is a mtan access which is technically public, but you shouldn't be able to see anything beyond what is displayed on the listing view. Take a look at NewsCollection/TopicCollection for how to handle access/publication restrictions.
| class PaginatedResourceCollection: | ||
|
|
||
| def __init__( | ||
| self, | ||
| resource_collection: ResourceCollection, | ||
| page: int = 0 | ||
| ) -> None: | ||
| self.resource_collection = resource_collection | ||
| self.session = resource_collection.session | ||
| self.page = page | ||
| self.batch_size = 25 | ||
|
|
||
| def by_id(self, id: PKType) -> Resource | None: | ||
| return self.resource_collection.by_id(id) # type: ignore | ||
|
|
||
| def subset(self) -> Query[Resource]: | ||
| return self.resource_collection.query().order_by(Resource.title) | ||
|
|
||
| @cached_property | ||
| def cached_subset(self) -> Query[Resource]: | ||
| return self.subset() | ||
|
|
||
| @property | ||
| def page_index(self) -> int: | ||
| return self.page | ||
|
|
||
| def page_by_index(self, index: int) -> Self: | ||
| return self.__class__( | ||
| self.resource_collection, page=index | ||
| ) | ||
|
|
||
| @property | ||
| def subset_count(self) -> int: | ||
| return self.cached_subset.count() | ||
|
|
||
| @property | ||
| def offset(self) -> int: | ||
| return self.page * self.batch_size | ||
|
|
||
| @property | ||
| def pages_count(self) -> int: | ||
| if not self.subset_count: | ||
| return 0 | ||
| return max(1, -(-self.subset_count // self.batch_size)) | ||
|
|
||
| @property | ||
| def batch(self) -> tuple[Resource, ...]: | ||
| return tuple( | ||
| self.cached_subset.offset(self.offset).limit(self.batch_size) | ||
| ) | ||
|
|
||
| @property | ||
| def previous(self) -> Self | None: | ||
| if self.page > 0: | ||
| return self.page_by_index(self.page - 1) | ||
| return None | ||
|
|
||
| @property | ||
| def next(self) -> Self | None: | ||
| if self.page < self.pages_count - 1: | ||
| return self.page_by_index(self.page + 1) | ||
| return None |
There was a problem hiding this comment.
I think you should be able to get rid of most of this code by using the Pagination mixin and the original non-paginated collection class. You will run into some mypy errors, because libres/onegov.reservation models use their own base class. But that should go away once the SQLAlchemy 2.0 changes have been merged.
| FormOrExternalLink = FormDefinition | ExternalFormLink | ||
| ResourceOrExternalLink = Resource | ExternalResourceLink |
There was a problem hiding this comment.
| FormOrExternalLink = FormDefinition | ExternalFormLink | |
| ResourceOrExternalLink = Resource | ExternalResourceLink | |
| FormOrExternalLink: TypeAlias = FormDefinition | ExternalFormLink | |
| ResourceOrExternalLink: TypeAlias = Resource | ExternalResourceLink |
This way these type aliases will be automatically rewritten to type statements by Ruff, once we migrate to Python 3.12.
| def batch(self) -> dict[ApiEndpointItem[FormOrExternalLink], | ||
| FormOrExternalLink]: | ||
| result: dict[ApiEndpointItem[FormOrExternalLink], | ||
| FormOrExternalLink] = {} | ||
| for item in self.collection.batch: | ||
| endpoint_item = self.for_item(item) | ||
| if endpoint_item: | ||
| result[endpoint_item] = item | ||
| for ext in self.session.query(ExternalFormLink).all(): | ||
| endpoint_item = self.for_item(ext) | ||
| if endpoint_item: | ||
| result[endpoint_item] = ext | ||
| return result |
There was a problem hiding this comment.
This isn't a great solution, external links should be properly mixed into the collection and respect the pagination page and limits, instead of getting appended to every page. This could get pretty hairy if we cared about the order of entries, since for a SQL UNION both queries need to share the same columns.
Luckily we don't particularly care about the order of entries here, so you may be able to emulate this by writing a generic PaginatedSumCollection helper which takes N collections and then transparently dispatches to the correct underlying collection(s) it is a sum off, based on the current page. The only slightly tricky part with be handling the offsets and limits in the second collection, since the first page will be shortened by however many entries there are in the final page of the first collection, so you can't just rely on each collection implementing Pagination, you will need to write some helpers for modifying the query of each collection, rather than being able to rely on their subset_count attributes etc.
On the plus side: The two sub-collections won't need to implement Pagination, so you can reuse the existing collections and you can reuse the PaginatedSumCollection for resources.
It might also be worth cutting down the total number of required queries from 2N to N+1 by emitting a single query for getting the total counts of all the collections using something like:
queries = [
collection.query().order_by(None).with_entities(
func.count(text('1')).label('count')
)
for collection in self.collections
]
query = queries[0]
for extra in queries[1:]:
query = query.union_all(extra)
return query.scalars()This would return a list of all collection counts in the same order as our collections, so you can determine which collection(s) you need to emit a query for and their corresponding limits based on the current page.
Note that you won't be able to rely on subet() and cached_subset in PaginatedSumCollection, so even though those attributes will exist since you inherit from Pagination, they should never be called.
2 participants
Commit message
Town6: Add API endpoints for forms, resources, people, meetings, political businesses and parliament
Registers 8 new API endpoints. Enriches form endpoint with text field for internal forms. Aligns PersonApiEndpoint with the agency model: 20 data fields, respects hidden_people_fields.
TYPE: Feature
LINK: -
Checklist