feat(api-v2): Add support for PDF files #1206
Conversation
|
I am in the office next week. So let me know if I can help. |
subotic
changed the title
|
This pull request has been mentioned on Discuss DaSCH. There might be relevant details there: https://discuss.dasch.swiss/t/support-non-image-files-in-knora-api-v2/33/1 |
# Conflicts: # webapi/src/test/scala/org/knora/webapi/e2e/v2/OntologyV2R2RSpec.scala
|
@subotic There's a test failure here because
So I increased the Redis connection timeout: |
|
@tobiasschweizer Can you review this? |
|
Ok, will do it now! |
| by adding IIIF parameters to `temporaryBaseIIIFUrl`. For example, to get | ||
| In the case of an image file, the client may now wish to get a thumbnail of each | ||
| uploaded image, to allow the user to confirm that the correct files have been uploaded. | ||
| This can be done by adding IIIF parameters to `temporaryBaseIIIFUrl`. For example, to get |
There was a problem hiding this comment.
Where does the mapping between Sipi's response and SipiUploadResponse happen? Is this a spray.json config?
There was a problem hiding this comment.
SipiUploadResponse is defined in KnoraSipiIntegrationV2ITSpec and is only used there. The client receives this response, and Knora never sees it.
There was a problem hiding this comment.
| override def toOntologySchema(targetSchema: OntologySchema): DocumentFileValueContentV2 = copy(ontologySchema = targetSchema) | ||
|
|
||
| override def toJsonLDValue(targetSchema: ApiV2Schema, projectADM: ProjectADM, settings: SettingsImpl, schemaOptions: Set[SchemaOption]): JsonLDValue = { | ||
| val fileUrl: String = s"${settings.externalSipiBaseUrl}/${projectADM.shortcode}/${fileValue.internalFilename}" |
There was a problem hiding this comment.
Could Knora also create the fragment identifier? Is it possible to refer to a part of a PDF in an annotation?
There was a problem hiding this comment.
It's not currently possible, but something could be added to knora-base for this.
There was a problem hiding this comment.
Could you make an issue for this?
| @@ -1298,31 +1298,31 @@ class ResourcesResponderV2(responderData: ResponderData) extends ResponderWithSt | |||
| resource: ReadResourceV2 = resources.toResource(gravsearchTemplateIri) | |||
|
|
|||
| _ = if (resource.resourceClassIri.toString != OntologyConstants.KnoraBase.TextRepresentation) { | |||
| throw BadRequestException(s"Resource $gravsearchTemplateIri is not a ${OntologyConstants.KnoraBase.XSLTransformation}") | |||
| throw BadRequestException(s"Resource $gravsearchTemplateIri is not a text file") | |||
There was a problem hiding this comment.
In this context, a Gravsearch template is expected.
There was a problem hiding this comment.
Clarified the error message in b4acacf.
| success, newFilePath = helper.filename_hash(fullImgName); | ||
| if not success then | ||
| server.sendStatus(500) | ||
| server.log(gaga, server.loglevel.LOG_ERR) |
There was a problem hiding this comment.
Was gaga even defined? ;-)
There was a problem hiding this comment.
Everything else wold have disappointed me ;-)
| return | ||
| end | ||
|
|
||
| --success, check = fullImg:mimetype_consistency(originalMimeType, originalFilename) |
There was a problem hiding this comment.
Where does Sipi check the actual mime type of the file?
There was a problem hiding this comment.
It doesn't in this Lua script; this code was already commented out, but I don't know why.
There was a problem hiding this comment.
I think it should be commented in again. Otherwise, the check is not performed.
There was a problem hiding this comment.
Then the integration test won't work anymore, I think. I commented it out. You are of course welcome to find a solution for the test that allows for the mime-type check to be added :-)
There was a problem hiding this comment.
I think there will also be situations where a problem cannot be fixed at once. But we should keep track of such problems by creating issues.
|
|
||
| server.log("upload.lua: wrote image file to " .. tmp_storage_file_path, server.loglevel.LOG_DEBUG) | ||
| else | ||
| -- It's not an image file. Just move it to its temporary storage location. |
There was a problem hiding this comment.
Can Sipi check that the mime type is correct?
There was a problem hiding this comment.
I think this is only implemented for SipiImage in Sipi. Is there a more generic implementation needed for any file type? The current implementation relies on libmagic.
There was a problem hiding this comment.
It would be better to have a more generic implementation. We could open an issue for this in Sipi.
There was a problem hiding this comment.
Yes, please do so and assign it to me.
|
In the logs (https://github.com/dasch-swiss/knora-api/pull/1206/checks?check_run_id=324265543) I see two 500 errors. Does this mean that |
Yes. Sipi says: I guess that's why that code was commented out. |
|
The file with the |
sipi/scripts/convert_from_path.lua
Outdated
| @@ -124,6 +124,20 @@ if media_type == IMAGE then | |||
| return | |||
| end | |||
|
|
|||
| local check | |||
| success, check = fullImg:mimetype_consistency(originalMimeType, originalFilename) | |||
There was a problem hiding this comment.
maybe you need to add
ocal submitted_mimetype
success, submitted_mimetype = server.parse_mimetype(originalMimeType)
local check
success, check = fullImg:mimetype_consistency(submitted_mimetype.mimetype, originalFilename)
There was a problem hiding this comment.
I've added that in a1a37cc, but Sipi returns the same HTTP 500 error. I actually think that since this is an API v1 or Sipi bug, and this PR only adds functionality to API v2, there's no reason to hold back this PR because of it.
There was a problem hiding this comment.
Ok , and you are going to change the v1 behaviour anyway. If your fix doesn't solve the problem, could you open an issue in Sipi?
There was a problem hiding this comment.
In other words, this problem was there before, and this PR doesn't affect it.
There was a problem hiding this comment.
I understand, but the code shouldn't have been just commented out in https://github.com/dasch-swiss/knora-api/pull/1459/files#diff-4a1a2aedd2d8b7dee2f824e33b243ca1R123
There was a problem hiding this comment.
It's ok for me to make separate issues (Knora and/or Sipi) and move on with this PR
There was a problem hiding this comment.
Perhaps, but that's a different PR. You can't expect every PR to fix every bug that already existed before the PR. You're reviewing this PR.
There was a problem hiding this comment.
Perhaps, but that's a different PR. You can't expect every PR to fix every bug that already existed before the PR. You're reviewing this PR.
I agree. But this is kind of a critical issue (Sipi is expected to recreate the image file in its original format), this is why I kept insisting. If it had been a detail, it would not have mattered that much.
|
It was commented out here: https://github.com/dasch-swiss/knora-api/pull/1459/files#diff-4a1a2aedd2d8b7dee2f824e33b243ca1R123 |
I think this is this the file that Knora temporarily stores and informs Sipi about so it can read the file. So this doesn't work any more as of Sipi 2.0.1. I do not understand why. |
|
Thanks for checking this very boring PR. :) |
|
.> Thanks for checking this very boring PR. :) I made me feel nostalgic ...good old api v1 times ... |
|
This pull request has been mentioned on Discuss DaSCH. There might be relevant details there: https://discuss.dasch.swiss/t/support-non-image-files-in-knora-api-v2/33/2 |
knora-base:pageCount,dimX, anddimYtoDocumentFileValue.originalFilenameandoriginalMimeTypeoptional,because we only get them for images.
knora-baseversion.knora-apicomplex ontology:documentFileValueHasPageCount,documentFileValueHasDimX,and
documentFileValueHasDimY.internalSipiFileServerGetUrlandexternalSipiFileServerGetUrlfrom
Settings, because all file types can now be stored together in Sipi.StandoffResponderV2to look for XSL transformations atinternalSipiBaseUrl(Sipi'sconfig.imgroot).get_mediatype.luawith a more flexible MIME-type-checkingframework in
file_info.lua.upload.lua.config.imgrootinstead ofconfig.docroot).KnoraSipiIntegrationV2ITSpec:Resolves #1202.
Fixes #1430.
Needs dasch-swiss/sipi#283.
Needs dasch-swiss/sipi#309.