-
Notifications
You must be signed in to change notification settings - Fork 16
fix: [PromptRegistry] Register new File converter in resttemplate
#660
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| package com.sap.ai.sdk.prompt.registry; | ||
|
|
||
| import java.io.File; | ||
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.io.OutputStream; | ||
| import java.nio.file.Files; | ||
| import javax.annotation.Nonnull; | ||
| import org.springframework.http.HttpInputMessage; | ||
| import org.springframework.http.HttpOutputMessage; | ||
| import org.springframework.http.MediaType; | ||
| import org.springframework.http.converter.AbstractHttpMessageConverter; | ||
| import org.springframework.http.converter.HttpMessageConverter; | ||
| import org.springframework.http.converter.HttpMessageNotReadableException; | ||
| import org.springframework.http.converter.HttpMessageNotWritableException; | ||
| import org.springframework.util.StreamUtils; | ||
|
|
||
| /** | ||
| * A custom implementation {@link HttpMessageConverter} for Spring's RestTemplate to read and write | ||
| * {@link java.io.File} objects in {@code application/octet-stream} payloads. | ||
| * | ||
| * @see org.springframework.http.converter.AbstractHttpMessageConverter | ||
| */ | ||
| class FileHttpMessageConverter extends AbstractHttpMessageConverter<File> { | ||
|
|
||
| FileHttpMessageConverter() { | ||
| super(MediaType.APPLICATION_OCTET_STREAM); | ||
| } | ||
|
|
||
| /** | ||
| * Indicates whether this converter supports the given class. | ||
| * | ||
| * <p>This implementation supports only {@link File}. | ||
| * | ||
| * @param clazz the target class to check | ||
| * @return {@code true} if and only if {@code clazz} is {@link File} | ||
| */ | ||
| @Override | ||
| protected boolean supports(@Nonnull final Class clazz) { | ||
| return File.class == clazz; | ||
| } | ||
|
|
||
| /** | ||
| * Reads the {@link HttpInputMessage} body into a new file in system's temporary directory. | ||
| * | ||
| * <p>The response body is streamed directly into this file without buffering the entire content | ||
| * in memory. | ||
| * | ||
| * <p>The caller is responsible for deleting the returned file. | ||
| * | ||
| * @param clazz the expected target class (always {@link File} | ||
| * @param inputMessage the HTTP message containing the response body | ||
| * @return a {@link File} containing the streamed response data | ||
| * @throws IOException if file creation or streaming fails | ||
| * @throws HttpMessageNotReadableException if the message cannot be read | ||
| */ | ||
| @Nonnull | ||
| @Override | ||
| protected File readInternal( | ||
| @Nonnull final Class<? extends File> clazz, @Nonnull final HttpInputMessage inputMessage) | ||
| throws IOException, HttpMessageNotReadableException { | ||
| final var tempFile = File.createTempFile("tmp", ".bin"); | ||
| try (OutputStream out = Files.newOutputStream(tempFile.toPath())) { | ||
| StreamUtils.copy(inputMessage.getBody(), out); | ||
| } | ||
| return tempFile; | ||
| } | ||
|
|
||
| /** | ||
| * Writes the contents of a {@link File} into the HTTP request body. | ||
| * | ||
| * <p>The file is streamed directly into the output message, avoiding unnecessary buffering. | ||
| * | ||
| * @param file the file whose contents should be written | ||
| * @param outputMessage the HTTP message whose body should be written to | ||
| * @throws IOException if reading the file or writing the body fails | ||
| * @throws HttpMessageNotWritableException if the message cannot be written | ||
| */ | ||
| @Override | ||
| protected void writeInternal( | ||
| @Nonnull final File file, @Nonnull final HttpOutputMessage outputMessage) | ||
| throws IOException, HttpMessageNotWritableException { | ||
| try (InputStream in = Files.newInputStream(file.toPath())) { | ||
| StreamUtils.copy(in, outputMessage.getBody()); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| package com.sap.ai.sdk.prompt.registry; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| import java.io.ByteArrayInputStream; | ||
| import java.io.ByteArrayOutputStream; | ||
| import java.io.File; | ||
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.io.OutputStream; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
| import javax.annotation.Nonnull; | ||
| import jdk.jfr.Description; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.io.TempDir; | ||
| import org.springframework.http.HttpHeaders; | ||
| import org.springframework.http.HttpInputMessage; | ||
| import org.springframework.http.HttpOutputMessage; | ||
|
|
||
| class FileHttpMessageConverterTest { | ||
|
|
||
| @Test | ||
| void testSupports() { | ||
| final var converter = new FileHttpMessageConverter(); | ||
|
|
||
| assertThat(converter.supports(File.class)).isTrue(); | ||
| assertThat(converter.supports(String.class)).isFalse(); | ||
| } | ||
|
|
||
| @Test | ||
| @Description("Test conversion from HttpInputMessage to File") | ||
| void testReadInternal() throws IOException { | ||
| final var converter = new FileHttpMessageConverter(); | ||
| final var messageContent = "Hello, World!".getBytes(); | ||
|
|
||
| final var inputMessage = | ||
| new HttpInputMessage() { | ||
| @Nonnull | ||
| @Override | ||
| public InputStream getBody() { | ||
| return new ByteArrayInputStream(messageContent); | ||
| } | ||
|
|
||
| @Nonnull | ||
| @Override | ||
| public HttpHeaders getHeaders() { | ||
| return new HttpHeaders(); | ||
| } | ||
| }; | ||
|
|
||
| final var generatedFile = converter.readInternal(File.class, inputMessage); | ||
| try { | ||
| assertThat(generatedFile).exists().isFile(); | ||
| assertThat(Files.readAllBytes(generatedFile.toPath())).isEqualTo(messageContent); | ||
| } finally { | ||
| Files.deleteIfExists(generatedFile.toPath()); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| @Description("Test conversion from File to HttpOutputMessage") | ||
| void testWriteInternal(@TempDir final Path tempDir) throws IOException { | ||
| final var converter = new FileHttpMessageConverter(); | ||
| final var fileContent = "Hello, World!".getBytes(); | ||
| final var tempFilePath = tempDir.resolve("testFile.txt"); | ||
| Files.write(tempFilePath, fileContent); | ||
|
|
||
| final var outputStream = new ByteArrayOutputStream(); | ||
| final var outputMessage = | ||
| new HttpOutputMessage() { | ||
|
|
||
| @Nonnull | ||
| @Override | ||
| public HttpHeaders getHeaders() { | ||
| return new HttpHeaders(); | ||
| } | ||
|
|
||
| @Nonnull | ||
| @Override | ||
| public OutputStream getBody() { | ||
| return outputStream; | ||
| } | ||
| }; | ||
|
|
||
| converter.writeInternal(tempFilePath.toFile(), outputMessage); | ||
| assertThat(outputStream.toByteArray()).isEqualTo(fileContent); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,27 @@ | ||
| package com.sap.ai.sdk.app.controllers; | ||
|
|
||
| import static java.nio.charset.StandardCharsets.UTF_8; | ||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| import com.fasterxml.jackson.databind.JsonNode; | ||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
| import com.fasterxml.jackson.dataformat.yaml.YAMLFactory; | ||
| import com.sap.ai.sdk.prompt.registry.model.PromptTemplate; | ||
| import com.sap.ai.sdk.prompt.registry.model.PromptTemplateDeleteResponse; | ||
| import com.sap.ai.sdk.prompt.registry.model.PromptTemplateListResponse; | ||
| import com.sap.ai.sdk.prompt.registry.model.PromptTemplatePostResponse; | ||
| import com.sap.ai.sdk.prompt.registry.model.PromptTemplateSubstitutionResponse; | ||
| import com.sap.ai.sdk.prompt.registry.model.SingleChatTemplate; | ||
| import java.io.IOException; | ||
| import java.nio.file.Files; | ||
| import java.util.List; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.springframework.core.io.ClassPathResource; | ||
|
|
||
| public class PromptRegistryTest { | ||
|
|
||
| static final ObjectMapper YAML_MAPPER = new ObjectMapper(new YAMLFactory()); | ||
|
|
||
| @Test | ||
| void listTemplates() { | ||
| var controller = new PromptRegistryController(); | ||
|
|
@@ -56,7 +64,13 @@ void importExportTemplate() throws IOException { | |
| PromptTemplatePostResponse template = controller.importTemplate(); | ||
| assertThat(template.getMessage()).contains("successful"); | ||
|
|
||
| // export TODO: NOT WORKING | ||
| // export | ||
| final var exportedTemplate = controller.exportTemplate(); | ||
|
|
||
| final var resource = new ClassPathResource("prompt-template.yaml"); | ||
| final JsonNode expectedYaml = YAML_MAPPER.readTree(resource.getContentAsString(UTF_8)); | ||
| assertThat(YAML_MAPPER.readTree(exportedTemplate)).isEqualTo(expectedYaml); | ||
|
Comment on lines
+67
to
+72
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The I can also instead resort to checking existence of some string within the returned file content. I just went for a complete equivalence check. |
||
| Files.deleteIfExists(exportedTemplate.toPath()); | ||
|
|
||
| // cleanup | ||
| List<PromptTemplateDeleteResponse> deletedTemplate = controller.deleteTemplate(); | ||
|
|
||
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.
(Question)
Is there life-cycle in Temp File? Do we need to delete this file? Does it exist physically on disk - or virtually in memory?
Uh oh!
There was an error while loading. Please reload this page.
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.
File.createTempFile()creates a physical file on disk in OS's temporary directory. My understanding was that if aFileobject is returned there needs to be a physical file it references.Uh oh!
There was an error while loading. Please reload this page.
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.
We cannot delete the
Fileand its upto the caller ofPromptTemplateApi.exportPromptTemplate()to do so.