fix(server): pass on multipart attachments without content type by supervacuus · Pull Request #3225 · getsentry/relay

supervacuus · 2024-03-06T12:37:07Z

This PR visualizes the change required to go forward with the approach suggested here: #3224

I introduced that set_payload() interface asymmetry only to avoid changing all the call sites for this PR.

supervacuus · 2024-03-06T17:30:51Z

A few integration tests fail due to assertions that expect application/octet-stream when no incoming content type was set. I can fix those, but whether this behavior should be changed is at the core of this issue/PR. Could you let me know if I should fix them anyway?

iker-barriocanal

Overall LGTM, and thanks for the context on the original issue!

Let's add some tests too, and it's good to go. (There are some tests using attachments in relay-server/src/envelope.rs, a similar structure is great.)

iker-barriocanal · 2024-03-12T13:30:16Z

relay-server/src/envelope.rs

    /// Sets the payload and content-type of this envelope.
-    pub fn set_payload<B>(&mut self, content_type: ContentType, payload: B)
-    where
+    pub(crate) fn set_payload_optional_content_type<B>(


The original set_payload does two things: set the content_type and truncate the payload. In the PR, the content type is only set from set_payload but never directly. Splitting both actions makes the Item's API usage easier to understand (vs wondering what a None is for). For example:

// envelope.rs fn set_payload(...) { self.headers.content_type = content_type; self.set_truncated_payload(...); } fn set_truncated_payload<B>(&mut self, payload: B) where B: Into<Bytes> { // ... } // multipart.rs if let Some(content_type) = field.content_type() { item.set_payload(content_type, field_data); } else { item.set_truncated_payload(field_data); }

(not opinionated on the function name.)

Extending the docstrings to point to the other function, depending on the content_type needs, would be great.

iker-barriocanal · 2024-03-12T13:34:58Z

relay-server/src/envelope.rs

        self.payload = payload;
    }

+    /// The primary payload setter requires a content-type, since most call-sites set it explicitly.


The original docstring is more descriptive:

Suggested change

/// The primary payload setter requires a content-type, since most call-sites set it explicitly.

/// Sets the payload and content-type of this envelope.

supervacuus · 2024-03-12T15:34:32Z

Let's add some tests too, and it's good to go. (There are some tests using attachments in relay-server/src/envelope.rs, a similar structure is great.)

Thanks, @iker-barriocanal.

To be clear, would you like to add attachment tests for multipart.rs analogous to the tests in envelope.rs? A test covering the multipart_items() content-type handling would be a good start.

Is it okay to assert the attachment's content-type properties for failing integration tests as None?

iker-barriocanal · 2024-03-13T15:25:58Z

To be clear, would you like to add attachment tests for multipart.rs analogous to the tests in envelope.rs? A test covering the multipart_items() content-type handling would be a good start.

Yep, test coverage for both branches of multipart_items based on content-type is good enough IMO.

Is it okay to assert the attachment's content-type properties for failing integration tests as None?

Yes, that's what we're looking for in the end -- no content-type should result in no value set, with no defaults.

Thanks for working on this, and let me know if you have further questions!

supervacuus · 2024-03-14T16:15:51Z

Hi @iker-barriocanal, I tried to cover the mentioned issues. I used a "real-life" multipart payload for the unit test and minimized the contents to data required for the assertions. It should cover all PR-touched paths in multipart_items() (and more), but it is written from the minidump perspective, so I added the test there. Let me know if I should add assertions/payload items or split the payload into multiple tests.

relay-server/src/utils/multipart.rs

…for the minidump.

…t_content_type

iker-barriocanal

Thanks for your effort and tests! One last question before merging 😄

iker-barriocanal · 2024-03-19T09:28:05Z

tests/integration/test_minidump.py

            "id": attachment_id,
            "name": "minidump.dmp",
-            "content_type": "application/octet-stream",
+            "content_type": None,


Q: Does sentry always expect the content_type field to exist?

Looking at this code maybe it does, but I'm not familiar with that part of the codebase. If it's ok to not include the field on null values, let's skip serializing it by adding a #[serde(skip_serializing_if = "Option::is_none")] to ChunkedAttachment.content_type on relay-server/src/services/store.rs.

+1, sentry might expect the content type, we should make sure it can handle missing / null values.

I am also not familiar with the code base. Still, when looking at the handling of attachments processed from envelope items (directly vs. via multipart), a non-existent content type appears to be valid input.

Let me check internally and I'll get back to you

It seems it's safe for content_type to be null or not exist (see sentry test), and sentry will also default to application/octet-stream if it can't guess the type.

@supervacuus let's skip the serialization when the field is empty as I posted on my comment above, and I'll merge the PR afterward.

relay-server/src/endpoints/minidump.rs

…t_content_type

iker-barriocanal

Thanks for putting in the effort and working on this!

I'll merge and deploy on Monday to avoid surprises on a Friday and weekend.

iker-barriocanal · 2024-03-25T16:21:39Z

There were some difficulties in the infrastructure today and this PR is not live. I'll make another attempt tomorrow and post an update here when the change is live.

iker-barriocanal · 2024-03-26T10:57:54Z

@supervacuus the PR is deployed!

supervacuus · 2024-03-26T14:33:50Z

@supervacuus the PR is deployed!

Thanks for the update. I tested this with a patched crashpad_handler; all attachments are correctly detected, and the preview works.

supervacuus requested a review from a team as a code owner March 6, 2024 12:37

supervacuus force-pushed the fix/multipart_passes_on_attachments_without_content_type branch 2 times, most recently from c751594 to d5469bf Compare March 6, 2024 14:10

supervacuus force-pushed the fix/multipart_passes_on_attachments_without_content_type branch from d5469bf to 4fbd340 Compare March 7, 2024 13:47

kahest mentioned this pull request Mar 7, 2024

Allow MIME detection for crashpad attachments in the backend getsentry/sentry-native#955

Closed

olksdr assigned iker-barriocanal Mar 11, 2024

iker-barriocanal suggested changes Mar 12, 2024

View reviewed changes

fix(server): pass on multipart attachments without content type

756c5c4

supervacuus force-pushed the fix/multipart_passes_on_attachments_without_content_type branch from 4fbd340 to 756c5c4 Compare March 14, 2024 15:21

supervacuus commented Mar 14, 2024

View reviewed changes

relay-server/src/utils/multipart.rs Outdated Show resolved Hide resolved

supervacuus and others added 2 commits March 15, 2024 14:54

Fix failing integration test because seemingly a content_type is set …

923fe4a

…for the minidump.

Merge branch 'master' into fix/multipart_passes_on_attachments_withou…

903a2ad

…t_content_type

iker-barriocanal reviewed Mar 19, 2024

View reviewed changes

supervacuus added 4 commits March 20, 2024 15:24

Clean up test variable and comments.

a1d1563

Go with set_payload_without_content_type()

eeff461

Skip serializing content_type of attachments if they are None.

70a262a

Merge branch 'master' into fix/multipart_passes_on_attachments_withou…

1482153

…t_content_type

iker-barriocanal approved these changes Mar 22, 2024

View reviewed changes

iker-barriocanal merged commit 4da9110 into getsentry:master Mar 25, 2024

supervacuus deleted the fix/multipart_passes_on_attachments_without_content_type branch March 25, 2024 13:56

This was referenced Mar 29, 2024

fix: send attachments without content type getsentry/crashpad#98

Merged

feat: enable automatic MIME detection for crashpad attachments. getsentry/sentry-native#973

Merged

p0358 mentioned this pull request Jul 30, 2024

No attachment preview for .log text files getsentry/sentry#38189

Closed

	/// The primary payload setter requires a content-type, since most call-sites set it explicitly.
	/// Sets the payload and content-type of this envelope.

Comments

Conversation

supervacuus commented Mar 6, 2024

Uh oh!

supervacuus commented Mar 6, 2024

Uh oh!

iker-barriocanal left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iker-barriocanal Mar 12, 2024

Choose a reason for hiding this comment

Uh oh!

iker-barriocanal Mar 12, 2024

Choose a reason for hiding this comment

Uh oh!

supervacuus commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iker-barriocanal commented Mar 13, 2024

Uh oh!

supervacuus commented Mar 14, 2024

Uh oh!

Uh oh!

iker-barriocanal left a comment

Choose a reason for hiding this comment

Uh oh!

iker-barriocanal Mar 19, 2024

Choose a reason for hiding this comment

Uh oh!

jjbayer Mar 19, 2024

Choose a reason for hiding this comment

Uh oh!

supervacuus Mar 20, 2024

Choose a reason for hiding this comment

Uh oh!

iker-barriocanal Mar 21, 2024

Choose a reason for hiding this comment

Uh oh!

iker-barriocanal Mar 21, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iker-barriocanal left a comment

Choose a reason for hiding this comment

Uh oh!

iker-barriocanal commented Mar 25, 2024

Uh oh!

iker-barriocanal commented Mar 26, 2024

Uh oh!

supervacuus commented Mar 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

iker-barriocanal left a comment •

edited

Loading

supervacuus commented Mar 12, 2024 •

edited

Loading