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

Add more headers in platform-http multipart uploads #17070

Merged
merged 2 commits into from
Feb 5, 2025

Conversation

Croway
Copy link
Contributor

@Croway Croway commented Feb 5, 2025

No description provided.

Copy link
Contributor

github-actions bot commented Feb 5, 2025

🌟 Thank you for your contribution to the Apache Camel project! 🌟

🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

  • First-time contributors require MANUAL approval for the GitHub Actions to run

  • You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot.

  • You can label PRs using build-all, build-dependents, skip-tests and test-dependents to fine-tune the checks executed by this PR.

  • Build and test logs are available in the Summary page. Only Apache Camel committers have access to the summary.

  • ⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

@@ -117,6 +117,7 @@ public interface Exchange extends VariableAware {
String CONTENT_TYPE = "Content-Type";
String COOKIE_HANDLER = "CamelCookieHandler";
String CORRELATION_ID = "CamelCorrelationId";
String ATTACHMENTS_SIZE = "CamelAttachmentsSize";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is somewhere you need to document this header, try look for some of the other headers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also put it up to A headers so its sorted

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah add it to ExchangePropertyKey also and doc via @Metadata

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmm, I cannot find much documentation for other headers, should I annotate it with Metadata and add a description?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/apache/camel/pull/17070/files#diff-c4832886119fc6dd6a4a54e68c5e2740ea9ae86b8699a469648284189d51e839 like this? I do not fully understand how ExchangePropertyKey works, for example, FILE* related headers are not there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Its the most commonly used headers - and since HTTP with attachments is fairly common then IMHO it would be okay to add here

Comment on lines 94 to 95
** The file's content type in the "CamelFileContentType" message header
** The file's size in the "CamelFileLength" message header
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick

Suggested change
** The file's content type in the "CamelFileContentType" message header
** The file's size in the "CamelFileLength" message header
** The file content type in the "CamelFileContentType" message header
** The file size in the "CamelFileLength" message header

@davsclaus
Copy link
Contributor

the size header should likely only be set if its > 0, as then it may be always there, also for just plain REST GET etc

@Croway
Copy link
Contributor Author

Croway commented Feb 5, 2025

the size header should likely only be set if its > 0, as then it may be always there, also for just plain REST GET etc

The ATTACHMENTS_SIZE header is added only during a multipart request. But I do not fully understand your comment.

@davsclaus
Copy link
Contributor

ah okay there was a if multipart check first - so this is fine. Just make sure on CSB that its also only added if really multipart data so its not there for anything else

@Croway
Copy link
Contributor Author

Croway commented Feb 5, 2025

ah okay there was a if multipart check first - so this is fine. Just make sure on CSB that its also only added if really multipart data so its not there for anything else

yup, same for CSB, all the logic is wrapped by https://github.com/apache/camel-spring-boot/blob/b358ea9050c281f550f7fc2cee12c055c5413371/components-starter/camel-platform-http-starter/src/main/java/org/apache/camel/component/platform/http/springboot/SpringBootPlatformHttpBinding.java#L98

@Croway Croway merged commit 928e6d2 into apache:main Feb 5, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants