Conversation
...gement/src/main/java/org/fiware/tmforum/documentmanagement/domain/DocumentSpecification.java
Show resolved
Hide resolved
|
|
||
| @EqualsAndHashCode(callSuper = true) | ||
| @MappingEnabled(entityType = DocumentSpecification.TYPE_DOCUMENT_SPECIFICATION) | ||
| public class DocumentSpecification extends EntityWithId { |
There was a problem hiding this comment.
The object lacks a numer of properties defined in the api. There is no:
- entitySpecRelationship
- specCharacteristic
- tragetEntitySchema
- validFor
There was a problem hiding this comment.
This is still incomplete, at least constraint is missing
...t-management/src/main/java/org/fiware/tmforum/documentmanagement/s3/S3AttachmentService.java
Show resolved
Hide resolved
...t-management/src/main/java/org/fiware/tmforum/documentmanagement/s3/S3AttachmentService.java
Outdated
Show resolved
Hide resolved
document-management/src/main/java/org/fiware/tmforum/documentmanagement/TMForumMapper.java
Outdated
Show resolved
Hide resolved
document-management/src/main/java/org/fiware/tmforum/documentmanagement/TMForumMapper.java
Outdated
Show resolved
Hide resolved
...-management/src/test/java/org/fiware/tmforum/documentmanagement/S3AttachmentServiceTest.java
Outdated
Show resolved
Hide resolved
...-management/src/test/java/org/fiware/tmforum/documentmanagement/S3AttachmentServiceTest.java
Show resolved
Hide resolved
...-management/src/test/java/org/fiware/tmforum/documentmanagement/S3AttachmentServiceTest.java
Show resolved
Hide resolved
|
|
||
| @EqualsAndHashCode(callSuper = true) | ||
| @MappingEnabled(entityType = DocumentSpecification.TYPE_DOCUMENT_SPECIFICATION) | ||
| public class DocumentSpecification extends EntityWithId { |
There was a problem hiding this comment.
This is still incomplete, at least constraint is missing
| .findFirst() | ||
| .ifPresent(att -> { | ||
| throw new TmForumException( | ||
| "Attachments with inline content are not supported when no S3 storage is configured. Provide a URL reference instead.", |
There was a problem hiding this comment.
Please change that to "when no AttachmentService is configured". This is not hard tied to S3
| DocumentSpecification.TYPE_DOCUMENT_SPECIFICATION))); | ||
|
|
||
| docSpec.setLastUpdate(clock.instant()); | ||
|
|
There was a problem hiding this comment.
Please simplify that those checks. Nested ifs should be avoided and its not necessary to check the attachment service being null multiple times
| */ | ||
| @PostConstruct | ||
| public void init() { | ||
| log.info("Initializing S3AttachmentService:"); |
There was a problem hiding this comment.
Why is this a multiline log? Makes it harder to parse for log-aggregators and more expensive to store
| log.info("S3 client created successfully"); | ||
| } catch (Exception e) { | ||
| log.error("Failed to create S3 client: {}", e.getMessage(), e); | ||
| throw new RuntimeException("Failed to initialize S3 client", e); |
There was a problem hiding this comment.
This is still a plain RuntimeException
| public Mono<HttpResponse<DocumentSpecificationVO>> patchDocumentSpecification( | ||
| String id, | ||
| DocumentSpecificationUpdateVO updateVO) { | ||
| // PATCH is not implemented per requirements (only GET/POST/DELETE) |
There was a problem hiding this comment.
Why is patch not required? Its part of the spec.
| assertEquals(HttpStatus.CREATED, response.getStatus(), message); | ||
| assertNotNull(response.body(), message); | ||
| assertNotNull(response.body().getId(), message); | ||
| assertEquals(expectedDocSpec.getName(), response.body().getName(), message); |
There was a problem hiding this comment.
This only validates correctness of the name. Why is this enough?
| assertFalse(response.body().getAttachment().isEmpty()); | ||
| // The attachment content should be S3 retrieval info, not the original content | ||
| String content = response.body().getAttachment().get(0).getContent(); | ||
| assertTrue(content.startsWith("s3ref:"), "Content should be S3 retrieval info"); |
There was a problem hiding this comment.
There is no S3RetrievalInfo in the code anymore. How can this test succeed?
| import static org.junit.jupiter.api.Assertions.*; | ||
| import static org.mockito.ArgumentMatchers.any; | ||
| import static org.mockito.Mockito.*; | ||
|
|
There was a problem hiding this comment.
The tests look better now, but please add messages to the assertions. There shouldn´t be any asserts without messages.
| <dependency> | ||
| <groupId>io.minio</groupId> | ||
| <artifactId>minio</artifactId> | ||
| <version>8.5.7</version> |
There was a problem hiding this comment.
Please configure the version in the top-level properties. This holds the risk of deviating versions between the aaio and the dedicated module
2 participants
Greetings @wistefan
This is the Document API management feature. I also added the changes to add this to the all-in-one feature recently added.
Best regards