-
Notifications
You must be signed in to change notification settings - Fork 67
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
Set Content-Type metadata in S3 from an Entity field annotated with @MimeType
#889
Set Content-Type metadata in S3 from an Entity field annotated with @MimeType
#889
Conversation
@@ -325,6 +327,8 @@ public class S3StoreIT { | |||
|
|||
BeforeEach(() -> { | |||
entity = new TestEntity(); | |||
entity.setContentType("application/content+json"); | |||
entity.setRenditionContentType("application/content+json"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately the test will fail, if I change thе second line to entity.setRenditionContentType("application/rendition+json"): because DefaultS3StoreImpl#setContent(S entity, InputStream content) must use BeanUtils.getFieldWithAnnotation that returns value of the first matching field and therefore "application/rendition+json" Content-Type is later stored for "content"-Resource too...
Is there something I can do here or using of ContentStore methods without PropertyPath on Entities with multiple fields managed by spring-content is generally not allowed (but it is done this way in other tests too, that most likely suffer from the same problem, which however is invisible because the same content string is set for both properties)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. Thanks. This is a bug in s3, azure and gcs storage modules. FS and JPA were ok. And azure and gcs wouldn't show the issue OOTB but it was still there. Anyways, I just merged this PR that should address that issue.
With that you should be able to rebase onto main and that test should go green. Let me know if you still have issues though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @paulcwarren , now everything works as expected! I did the rebase onto main and tweaked my changes a bit.
However, I still have a question about the correct mapping of the required fields e.g. in DefaultS3StoreImpl#setContent(S entity, InputStream content), that is in the methods where PropertyPath is not explicitly specified and BeanUtils is used. Maybe it's just luck here that BeanUtils methods always manipulate fields related to "content"-Resource (most likely the alphabetically first field with the required annotation is accessed, because if I rename fields content{Id,Len,ContentType} to storedContent{Id,Len,ContentType} many tests will fail). Did I understand it correctly?
And if so, it turns out that using methods without explicitly specifying PropertyPath on Еntities that have multiple fields managed by spring-content, as it is done in this test, is actually undesirable and should perhaps even fail with Exception (since the outcome of these method calls is somewhat non-obvious, although in this case it accidentally leads to the right result)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those (legacy) methods are intended to be used with entities with a single content property only. I agree this is not called out well enough in the documentation. I'll try and improve this. We could also consider deprecating them. Spring Content Rest module uses the new variants only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification, @paulcwarren, because I saw an example of such usage in tests I doubted my understanding:-)
Methods without PropertyPath specification are surely convenient, if entity is associated with only one content property... So, I think they should be kept (also for backward compatibility), but I think it's easy to accidentally add one more content property and forget to update ContentStore methods and get weird bugs. So it seems to me that checking for the presence of multiple content-properties in Entity and throwing an exception inside methods without PropertyPath would also make sense, even if it will be mentioned in documentation, that it is not allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah sorry about that. I didn't have time to refactor the tests. I have a lot of them! But that does kinda give the wrong impression. I like your suggestion of throwing an exception. That should be easy to implement too (can check the length of content properties for the entity) except then I really would have to refactor some tests!
Thanks for the PR @kreymerman . I'll look at it as soon as I can. |
699a82c
to
c363a73
Compare
Possible solution that fixes #67