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

Encoded RequestParam values increase the number of "self link...don't match" logs #3045

Closed
tdonohue opened this issue May 14, 2024 · 2 comments · Fixed by #3046
Closed

Encoded RequestParam values increase the number of "self link...don't match" logs #3045

tdonohue opened this issue May 14, 2024 · 2 comments · Fixed by #3046
Assignees
Labels
bug testathon Reported by a tester during Community Testathon
Milestone

Comments

@tdonohue
Copy link
Member

tdonohue commented May 14, 2024

Describe the bug
After merging #2725, I've noticed that PR had the side effect of increasing these statements in the UI logs:

The response for 'http://localhost:8080/server/api/authz/authorizations/search/object?uri=http%3A%2F%2Flocalhost%3A8080%2Fserver%2Fapi%2Fcore%2Fsites%2F6d65c6a2-3fe7-44dd-bacb-79271257c35d&feature=canSubmit' 
has the self link 'http://localhost:8080/server/api/authz/authorizations/search/object?uri=http://localhost:8080/server/api/core/sites/6d65c6a2-3fe7-44dd-bacb-79271257c35d&feature=canSubmit'. 
These don't match. This could mean there's an issue with the REST endpoint

The reason is that the UI is comparing one URL that has the RequestParam values all encoded, to one which does NOT have those params encoded. If you look closely, in the statement above, the URLs are identical except for the encoded params.

Originally posted by @tdonohue in #2725 (comment)

To Reproduce
Steps to reproduce the behavior:

  1. Ensure you are on latest main.
  2. Simply visit the homepage. You'll see a number of these self link messages appear in the UI logs. The self link actually DOES match the URL, but a simple string comparison fails because of the encoded parameters.
@github-project-automation github-project-automation bot moved this to 🆕 Triage in DSpace Backlog May 14, 2024
@tdonohue tdonohue added bug testathon Reported by a tester during Community Testathon labels May 14, 2024
@tdonohue tdonohue added the help wanted Needs a volunteer to claim to move forward label May 14, 2024
@tdonohue
Copy link
Member Author

@alexandrevryghem : Pinging you on this new ticket as it seems to be a side effect of #2725. One simple solution might be to change the logic in this area of the code to do the comparison with both URLs encoded or both URLs decoded:

https://github.com/DSpace/dspace-angular/blob/main/src/app/core/data/dspace-rest-response-parsing.service.ts#L163-L172

Let me know if you feel you can claim this.

@alexandrevryghem
Copy link
Member

@tdonohue: Ideally the uri parameter should also be encoded in the backend, but this would be a bigger change. Instead I would maybe simply not encode the uri param in the URL. This was how it worked before PR #2725, because updating the code in DspaceRestResponseParsingService to always decode all the parameters could cause other issues I think:

  • If you encode all the params from the self link that is returned from the backend, you may potentially encode an already encoded parameter value twice, which will cause them not to match anymore
  • If you decode all the params from the request URL that you sent, and that the backend returned a self link with encoded params they might also not match anymore

@alexandrevryghem alexandrevryghem self-assigned this May 14, 2024
@alexandrevryghem alexandrevryghem removed the help wanted Needs a volunteer to claim to move forward label May 14, 2024
@github-project-automation github-project-automation bot moved this from 📋 To Do to ✅ Done in DSpace 8.0 Release May 15, 2024
@tdonohue tdonohue moved this from 📋 To Do to ✅ Done in DSpace 8.x and 7.6.x Maintenance May 15, 2024
@tdonohue tdonohue added this to the 7.6.2 milestone May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug testathon Reported by a tester during Community Testathon
Projects
Development

Successfully merging a pull request may close this issue.

2 participants