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

feat(sipi): Improve error message if XSL file not found #1590

Merged
merged 7 commits into from Feb 7, 2020

Conversation

@benjamingeer
Copy link
Collaborator

@benjamingeer benjamingeer commented Feb 5, 2020

This PR improves the error returned if Knora requests a nonexistent XSL transformation file from Sipi.

Resolves #1580.

@benjamingeer benjamingeer added the API/V2 label Feb 5, 2020
@benjamingeer benjamingeer added this to the 2020-02 milestone Feb 5, 2020
@benjamingeer benjamingeer self-assigned this Feb 5, 2020
@benjamingeer benjamingeer mentioned this pull request Feb 5, 2020
benjamingeer added 2 commits Feb 5, 2020
@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 5, 2020

@flavens Could you check if this branch gives you a better error message?

@flavens
Copy link

@flavens flavens commented Feb 5, 2020

@flavens Could you check if this branch gives you a better error message?

I get back this message:
Screenshot 2020-02-05 at 17 03 37

I think it is better, however, I would have search for a long time it was the BEOL server file at the wrong place! I still need more experience with errors to get a better instinct :)

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 5, 2020

@flavens Hmm, that's not what it's supposed to say. Can you give me a simple way to make this error happen?

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 5, 2020

It's supposed to say something like "Unable to get XSL transformation file XYZ from Sipi"...

@flavens
Copy link

@flavens flavens commented Feb 6, 2020

@flavens Hmm, that's not what it's supposed to say. Can you give me a simple way to make this error happen?

You can checkout the master branch of Knora-app.

Run on your terminal: yarn install --prod=false + ng s (you have to install yarn and angular-cli)

Then go to http://0.0.0.0:4200/system/users after log in (you can use the root user profile) and try to Add a user as system admin:

Screenshot 2020-02-06 at 08 26 37

See error in the console.

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 6, 2020

% yarn install --prod=false + ng s
yarn install v1.22.0
error `install` has been replaced with `add` to add new dependencies. Run "yarn add + ng s" instead.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 6, 2020

% yarn add --prod=false + ng s
yarn add v1.22.0
[1/4] 🔍  Resolving packages...
error An unexpected error occurred: "https://registry.yarnpkg.com/+: Not found".
info If you think this is a bug, please open a bug report with the information provided in "/Users/benjamingeer/git/knora-app/yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/add for documentation about this command.
@flavens
Copy link

@flavens flavens commented Feb 6, 2020

% yarn add --prod=false + ng s
yarn add v1.22.0
[1/4] 🔍  Resolving packages...
error An unexpected error occurred: "https://registry.yarnpkg.com/+: Not found".
info If you think this is a bug, please open a bug report with the information provided in "/Users/benjamingeer/git/knora-app/yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/add for documentation about this command.

you run yarn **install** not yarn add first (make sure yarn is installed) and then, ng s(with angular-cli installed on your computer)
No combination of the 2 commands

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 6, 2020

you run yarn install not yarn add first

I did that and got an error message that told me to run yarn add instead, as shown in my comment above:

#1590 (comment)

The error message says "error install has been replaced with add to add new dependencies. Run "yarn add + ng s" instead."

But yarn add doesn't work either, it gives a different error message.

@musicEnfanthen
Copy link
Collaborator

@musicEnfanthen musicEnfanthen commented Feb 6, 2020

Can confirm that the whole command yarn install --prod=false + ng s is not working. You would have to run it sequentially:

yarn install --prod=false

[wait for yarn to complete installation]

ng s [optional with ng s -o, then the app opens directly in the browser]

EDIT: It has to be install not add to install the whole list of dependencies. Add just installs a single package specified in the add command. So in the case of your second not working command (yarn add --prod=false + ng s), it would try to add the three packages +, ng and s which are obviously not existing.

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 6, 2020

@musicEnfanthen Thanks, I just figured out the same thing at the same time. :)

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 6, 2020

@flavens I did what you described, and I can see in the console that the request is empty ({}). That's why you're getting the error "The request content was malformed: No data sent in API request." Maybe that's because of a bug in knora-app.

In any case, I don't think that can have anything to do with the issue about an XSL file that doesn't exist. The request to create an admin user uses the admin API, which doesn't do anything with XSL templates. XSL templates are used to transform text markup in a resource.

@flavens
Copy link

@flavens flavens commented Feb 6, 2020

@benjamingeer ah! an idea: I spotted this error before the release of knora-app. In the master branch, we have the newest knora-api-js-lib. To reproduce the error, you could try with knora-api-js-lib v0.1.1, knora-api v11.0.0 and place the BEOL server file in another folder.

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 6, 2020

@flavens OK, I've actually just added an e2e test for this, which confirms that if you request a resource with an XSL transformation, and the XSL file doesn't exist, you get an HTTP 500 error with the message "Unable to get XSL transformation file".

I don't know how to check this manually using the BEOL data, but I guess you would have to request a BEOL resource. @tobiasschweizer can you help?

@benjamingeer benjamingeer requested review from tobiasschweizer and flavens Feb 6, 2020
responseStr <- doSipiRequest(request)
} yield SipiGetTextFileResponse(responseStr)

sipiResponseTry.recover {
case badRequestException: BadRequestException => throw SipiException(s"Unable to get XSL transformation file $xsltFileUrl from Sipi: ${badRequestException.message}")

This comment has been minimized.

@tobiasschweizer

tobiasschweizer Feb 6, 2020
Contributor

sipiGetXsltTransformationRequestV2 is called in two cases: for XSLT used by StandoffResponderV2 to turn XML into HTML and for XSLT used the TEI transformer in ResourcesResponderV2. So I guess testing it with the TEI route is fine.

However, SipiGetTextFileRequest is as the message sent to SipiConnector that then internally calls sipiGetXsltTransformationRequestV2 seems strange to me. Shouldn't either the message be more specific or the method more generic?

This comment has been minimized.

@tobiasschweizer

tobiasschweizer Feb 6, 2020
Contributor

In the future, Knora might require other text files from Sipi than XSL transformations and then the error message thrown by sipiGetXsltTransformationRequestV2 might be misleading.

This comment has been minimized.

@benjamingeer

benjamingeer Feb 6, 2020
Author Collaborator

Quite true, I'll fix that.

This comment has been minimized.

@benjamingeer

benjamingeer Feb 7, 2020
Author Collaborator

Done in 080dd44.

@subotic subotic modified the milestones: 2020-02, 2020-01 Feb 7, 2020
@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 7, 2020

@tobiasschweizer @flavens Is this better now?

Copy link
Contributor

@tobiasschweizer tobiasschweizer left a comment

Ok, I think like this it is much better.

Is there a way to tell the client why Knora tried to get a text file from Sipi? There are two cases at the moment : one in the TEI route and one in the standoff route.

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 7, 2020

Is there a way to tell the client why Knora tried to get a text file from Sipi?

What would you like it to say?

@tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Feb 7, 2020

What would you like it to say?

It would be great if the message would tell who sent the message to SipiResponder, e.g. the StandoffResponderV2.

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 7, 2020

You'd like it to say, "Requested by StandoffResponderV2"?

@tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Feb 7, 2020

You'd like it to say, "Requested by StandoffResponderV2"?

Yes, but only if the message was sent by StandoffResponder. Can you find out about this in SipoResponder?

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 7, 2020

Can you find out about this in SipoResponder?

I could add it to the message.

@tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Feb 7, 2020

I could add it to the message.

That would great. It is like a stack trace.

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 7, 2020

OK, now the error message is "Unable to get file http://0.0.0.0:1024/0801/missing.xsl from Sipi as requested by org.knora.webapi.responders.v2.StandoffResponderV2: Sipi responded with HTTP status code 404: Not Found".

Copy link
Contributor

@tobiasschweizer tobiasschweizer left a comment

That's great, thanks!

@flavens
flavens approved these changes Feb 7, 2020
@benjamingeer benjamingeer merged commit bbb42f6 into develop Feb 7, 2020
7 checks passed
7 checks passed
Build Everything
Details
API Unit Tests
Details
API E2E Tests
Details
API Integration Tests
Details
Upgrade Unit Tests
Details
Upgrade Integration Tests
Details
Docs Build Test
Details
@benjamingeer benjamingeer deleted the wip/1580-xsl-not-found branch Feb 7, 2020
@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 7, 2020

@tobiasschweizer @flavens Thanks to both of you for your help with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

5 participants