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

@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 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
Copy link
Author

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

@flavens
Copy link

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
Author

@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
Author

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

@flavens
Copy link

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
Author

% 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
Author

% 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 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
Author

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 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
Author

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

@benjamingeer
Copy link
Author

@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 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
Author

@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?

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}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite true, I'll fix that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 080dd44.

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

@tobiasschweizer @flavens Is this better now?

Copy link
Contributor

@tobiasschweizer tobiasschweizer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Author

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 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
Author

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

@tobiasschweizer
Copy link
Contributor

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
Author

Can you find out about this in SipoResponder?

I could add it to the message.

@tobiasschweizer
Copy link
Contributor

I could add it to the message.

That would great. It is like a stack trace.

@benjamingeer
Copy link
Author

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great, thanks!

@benjamingeer benjamingeer merged commit bbb42f6 into develop Feb 7, 2020
@benjamingeer benjamingeer deleted the wip/1580-xsl-not-found branch February 7, 2020 15:53
@benjamingeer
Copy link
Author

@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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve error message with more details about the failure
5 participants