bluesky-social / atproto

Social networking technology created by Bluesky
Other
6.88k stars 486 forks source link

Video Service issues #3026

Open MarshalX opened 4 days ago

MarshalX commented 4 days ago

Describe the bug

In Python SDK we have send_video since early days of video support. But this methods utilize uploadBlob method. Which performs all communication with video service under the hood. I really appreciate that you introduced such an easy method to start! But this method has lack of access to processing errors. And it started the common issue for our Python community to ask why "video not found" happens. That's why I started writing the complete example of how to use Video Service directly with job status handling and etc. During the development, I faced with a few really strange things that I want to raise.

I have split this issue to 3 sections. Each represents own problem.

1. Server response is out of the lexicon

To Reproduce

Upload video using uploadVideo endpoint of Video Service

image

You will see the response like this:

image

The response misses jobStatus root object described in the lexicon.

Expected behavior

https://github.com/bluesky-social/atproto/blob/c34426fc55e8b9f28d9b1d64eab081985d1b47b5/lexicons/app/bsky/video/uploadVideo.json#L11-L23

jobStatus is required field which should be this definition:

https://github.com/bluesky-social/atproto/blob/c34426fc55e8b9f28d9b1d64eab081985d1b47b5/lexicons/app/bsky/video/defs.json#L2-L5

2. Server status code responses

To Reproduce

Use uploadVideo again with the same video file.

HTTP 409 with Video processed successfully is a bit wild and IMO does not fit XRPC ideology of status codes (https://atproto.com/specs/xrpc#summary-of-http-status-codes)

image

Expected behavior

It is getting really hard to work with. As it requires try-catch for 409 and additional error content deserialization to the proper model. Which is out of the default flow when we parse models in succeed response. It would be really helpful to see the status code from 2xx group.

3. Input of the uploadVideo method is described in the lexicon partially only

To Reproduce

Try to use uploadVideo only with the info described in the lexicon

https://github.com/bluesky-social/atproto/blob/c34426fc55e8b9f28d9b1d64eab081985d1b47b5/lexicons/app/bsky/video/uploadVideo.json#L8-L10

You will be rejected by the server validation that you are missing DID or name input data.

Expected behavior

Probably this could be unique situation when the procedure has both input and parameters, but at least it should be described somehow in the lexicon (IMO). My understanding of missing parameters comes from the traffic sniffing of the official app...

image

Not an issue, just a question

Any participial reason to make name parameter required? As I can see it is randomly generated with the same extension of the uploaded file. Could be easily done on server side? And the DID could be obtained from the service token? Am I wrong?

P.S. I am sorry for being so direct ❣️ I am understand that Video Service is like 3d party service and could do not follow the best practices of atproto, but since is it developed by the same developers I expected more smooth work with it. I just shared my experience. Happy to hear your thoughts. Thank you!