Skip to content

Conversation

@kyokukou
Copy link
Contributor

@kyokukou kyokukou commented Jan 6, 2025

No description provided.

@kyokukou kyokukou requested a review from bdc34 January 6, 2025 15:14
.filter(Metadata.document_id.in_(selected_doc_ids.select()),
Metadata.is_current == 1
)
.options(load_only(
Copy link

Choose a reason for hiding this comment

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

FYI I think that .options can be called later so you can have the same query but with different load_only options.

def fetch_list(just_ids:bool, start_date :datetime, end_date:datetime, meta_type:MetadataFormat, rq_set:Optional[Union[Group, Archive, Category]], skip: int, query_data: Dict[OAIParams, str])-> Response:
"""fetches the required data for a record for a specific format
converts data into specific format and renders record template
"""
Copy link

@bdc34 bdc34 Jan 6, 2025

Choose a reason for hiding this comment

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

Should start_date and end_date have timezones? not have timezones? are fine with or without?

Add this info to the docstr.


#resumption token handling
res_token=None
if len(objects)>limit:
Copy link

Choose a reason for hiding this comment

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

I'm concerned here that a query might bring back a very large number of objects and load them into memory.

What is preventing a client from requesting a very large result and then only here it gets limited to smaller set with a resumption token?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the database query is set with .limit(limit+1) so it only retrieves one extra item which is used as a check to see if there are still more

@bdc34 bdc34 self-requested a review January 6, 2025 19:33
now removes paper with the latest timestamp from list instead of last in list.
added tests for chaining resumption tokens.
changed dates to rely soley on modtime stamp.
@kyokukou kyokukou added this pull request to the merge queue Jan 9, 2025
Merged via the queue into develop with commit f365ddf Jan 9, 2025
3 checks passed
@kyokukou kyokukou deleted the list_records branch January 9, 2025 21:18
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.

3 participants