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: remove default for type field of the Attachment class #169

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

adubovik
Copy link
Collaborator

@adubovik adubovik commented Oct 16, 2024

The SDK defaults a missing type field to text/markdown:

class Attachment(ExtraForbidModel):
type: Optional[StrictStr] = "text/markdown"
title: Optional[StrictStr] = None
data: Optional[StrictStr] = None
url: Optional[StrictStr] = None
reference_type: Optional[StrictStr] = None
reference_url: Optional[StrictStr] = None

This non-trivial default makes it impossible to receive attachments like this one:

{ "url": "https://example.com/foo/bar/doc.pdf"}

Because it will be parsed to the object:

Attachment(url="https://example.com/foo/bar/doc.pdf", type="text/markdown")

which is utterly bizarre and hard to deal with in the applications.
See how it's worked around in the adapter-bedrock, likewise in adapter-vertexai.

Essentially, the parser from JSON to Attachment

  1. reinterprets the data from the user as something that it isn't, and
  2. obscures the data, because the downstream code doesn't see what exactly came in the request.

The proposal is to default to None.

On the other hand, SDK provides inconsistent ways to create Attachment objects.

See helpers at aidial_sdk.utils._attachment:

>>> print(create_attachment(Attachment()))
type='text/markdown' title=None data=None url=None reference_type=None reference_url=None
>>> print(create_attachment())
type=None title=None data=None url=None reference_type=None reference_url=None

@adubovik adubovik self-assigned this Oct 16, 2024
@adubovik adubovik changed the title fix: removed default for an attachment type fix: remove default for an attachment type Nov 29, 2024
@adubovik adubovik changed the title fix: remove default for an attachment type fix: remove default for type field of the Attachment class Nov 29, 2024
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.

1 participant