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

[NHUB-540] Add async ResourceModel for ContentAPI items #2740

Merged
merged 14 commits into from
Oct 30, 2024

Conversation

MarkLark86
Copy link
Contributor

Purpose

Create async ResourceModel for ContentAPI items, and implement publish code to it as simple functions (for re-use with Newshub). Also small changes required by the Newshub Wire async upgrade task.

What has changed

  • Create async ResourceModel for ContentAPI items
  • Create re-usable functions for publishing into the ContentAPI (for re-use with Newshub)
  • Support projection argument to async find_one_ based functions
  • Catch more Elasticsearch search errors, due to invalid search request, and print the query as well
  • Support nested include_in_parent elasticsearch config
  • Create a new BaseModel from Pydantic, so we have common interface to all models, regardless of usage (resource model, view args etc).
  • ElasticCursor: Support asycn next function, also reset elastic iterator index once iteration completed (so it can be iterated again, if needed)
  • Support more elasticsearch mapping settings on fields/annotations
  • Fix issue with async cacheable service, locking in threads was causing an issue (in tests only I believe). So provide fallback so the function will always work
  • Add a TODO comment to validators, support validate_iunique_value_async without requiring to add resource_name and possibly field_name, making it easier to use. Will do this later
  • Improve layout of the types module, moving out definitions to different files, making it easier to maintain
  • Add is_json_request to Request instance
  • Add GUID_FIELD to configured lookup fields (in superdesk.resource_fields)

@MarkLark86 MarkLark86 added this to the 3.0 milestone Oct 28, 2024
@MarkLark86 MarkLark86 requested a review from eos87 October 28, 2024 05:04
@MarkLark86 MarkLark86 changed the title Nhub 540 [NHUB-540] Add async ResourceModel for ContentAPI items Oct 28, 2024
Copy link

@eos87 eos87 left a comment

Choose a reason for hiding this comment

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

Looks good, just left two minor comments.

planning_id: fields.Keyword | None = None
coverage_id: fields.Keyword | None = None
agenda_id: fields.Keyword | None = None
agenda_href: fields.Keyword | None = None
Copy link

Choose a reason for hiding this comment

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

I'm seeing so many of these fields.Keyword | None = None. Would it make sense to have something like this:

    # first define a optional keyword field type
    OptionalKeyword = Annotated[fields.Keyword | None, Field(default=None)]

    # and then reuse it like
    event_id: OptionalKeyword
    planning_id: OptionalKeyword
    coverage_id: OptionalKeyword
    # and so on

I didn't test this, it's just a thought. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this and MyPy does't like it, it thinks that no default value is supplied and complains when another field with an = None before if:
Attributes without a default cannot follow attributes with one

Comment on lines +37 to +59
media: Any

mail: Any

data: Any

storage: Any

auth: Any

subjects: Any

notification_client: NotificationClientProtocol

locators: Any

celery: Any

redis: Any

jinja_loader: Any

jinja_env: Any
Copy link

Choose a reason for hiding this comment

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

are we going to provide types for these in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally we would not use this at all, by replacing the required functionality into the new async app. For example, data will no longer be required as we have equivalent in the new async app.

@MarkLark86 MarkLark86 merged commit beb6af3 into superdesk:async Oct 30, 2024
8 of 12 checks passed
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