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(api-v2): Remove ForbiddenResource #1615

Merged
merged 31 commits into from Apr 7, 2020
Merged

Conversation

@benjamingeer
Copy link
Collaborator

@benjamingeer benjamingeer commented Feb 28, 2020

User story: https://dasch.myjetbrains.com/youtrack/issue/DSP-36

Discussion:

https://discuss.dasch.swiss/t/how-to-deal-with-forbidden-resources-and-values-consistently/153

https://discuss.dasch.swiss/t/knora-api-js-lib-pr-changing-public-api/158/5

This PR removes ForbiddenResource to make Gravsearch more consistent with the rest of API v2.

Instead of using ForbiddenResource, Gravsearch now returns "knora-api:mayHaveMoreResults": true if the number of main resources returned, plus the number of main resources hidden because of permissions, equals the page size.

This has a couple of other advantages:

  • The client doesn't need to know the maximum page size (which is a server configuration setting that the client doesn't have access to) to find out if it can request another page.
  • The response is more concise.

The rest of API v2 now also distinguishes between a resource not being found (NotFoundException, HTTP 404) and the user not having permission to see it (ForbiddenException, HTTP 403).

This PR also unifies the processing of search responses and other resource responses in ConstructResponseUtilV2.

Tasks:

  • Change knora-base.ttl and system-data.ttl.
  • Change ConstructResponseUtilV2 and its dependencies.
  • Update tests and test data.
  • Add a client test response that includes knora-api:mayHaveMoreResults.
  • Update API docs.
  • Update design docs.
  • Increase the knora-base version number.

Resolves #1543.

# Conflicts:
#	webapi/src/main/scala/org/knora/webapi/util/StringFormatter.scala
@benjamingeer benjamingeer mentioned this pull request Feb 28, 2020
@benjamingeer benjamingeer self-assigned this Feb 28, 2020
@benjamingeer benjamingeer added this to the 2020.1 milestone Feb 28, 2020
@daschbot
Copy link
Collaborator

@daschbot daschbot commented Mar 2, 2020

This pull request has been mentioned on Discuss DaSCH. There might be relevant details there:

https://discuss.dasch.swiss/t/how-to-deal-with-forbidden-resources-and-values-consistently/153/1

@benjamingeer benjamingeer changed the title feat(api-v2): Use ForbiddenResource consistently feat(api-v2): Get rid of ForbiddenResource Mar 5, 2020
@benjamingeer benjamingeer changed the title feat(api-v2): Get rid of ForbiddenResource feat(api-v2): Remove ForbiddenResource Mar 5, 2020
@daschbot
Copy link
Collaborator

@daschbot daschbot commented Mar 6, 2020

This pull request has been mentioned on Discuss DaSCH. There might be relevant details there:

https://discuss.dasch.swiss/t/how-to-deal-with-forbidden-resources-and-values-consistently/153/7

@benjamingeer benjamingeer requested a review from tobiasschweizer Mar 6, 2020
- Simplify Gravsearch paging.
- Add design docs.
@daschbot
Copy link
Collaborator

@daschbot daschbot commented Mar 26, 2020

This pull request has been mentioned on Discuss DaSCH. There might be relevant details there:

https://discuss.dasch.swiss/t/knora-api-js-lib-pr-changing-public-api/158/2

@daschbot
Copy link
Collaborator

@daschbot daschbot commented Mar 27, 2020

This pull request has been mentioned on Discuss DaSCH. There might be relevant details there:

https://discuss.dasch.swiss/t/knora-api-js-lib-pr-changing-public-api/158/5

# Conflicts:
#	docs/src/paradox/05-internals/design/api-v2/gravsearch.md
#	webapi/src/main/scala/org/knora/webapi/responders/v2/SearchResponderV2.scala
@benjamingeer benjamingeer requested a review from SepidehAlassi Apr 3, 2020
@daschbot
Copy link
Collaborator

@daschbot daschbot commented Apr 6, 2020

This pull request has been mentioned on Discuss DaSCH. There might be relevant details there:

https://discuss.dasch.swiss/t/knora-api-js-lib-pr-changing-public-api/158/9

Copy link
Contributor

@SepidehAlassi SepidehAlassi left a comment

Please resolve the failing test related to JS-Lib. As you have mentioned in the "Proposal: Analysis of Dependencies ..." document,

No subsystem should have to delay merging a PR until some other system can support that PR.

I suggest that, in another branch/PR, you either remove these tests or replace them with tests to check the compatibility of releases, not the compatibility of commits, as you suggested yourself. After that PR is merged to the develop branch, with a control merge, the problem of failing tests of this PR must be solved.


val transformedValuePropertyAssertions: RdfPropertyValues = resource.valuePropertyAssertions.map {
case (propIri, values) =>
val transformedValues = values.map {
value =>
val transformedValues: Seq[ValueRdfData] = values.foldLeft(Vector.empty[ValueRdfData]) {

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Apr 6, 2020
Contributor

Please extract this foldleft operation into a function to increase readability. It is 26 lines with 3 levels of indentation (nested conditions)

This comment has been minimized.

@benjamingeer

benjamingeer Apr 7, 2020
Author Collaborator

The whole splitMainResourcesAndValueRdfData method has always been a headache for me to read: too long and too much nesting. Each time I have to work on it, I try to make it a bit clearer, but it really needs to be split up into much smaller pieces. I was going to ask you to refactor it with Tobias, since he's the author. :) For now, I've just moved the nestResources method out to the top level, and I've moved the foldLeft operation into another function, transformValuesByNestingResources (in 6e96460). But even after doing that, splitMainResourcesAndValueRdfData is still 273 lines long. :(

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Apr 7, 2020
Contributor

For now, I've just moved the nestResources method out to the top level, and I've moved the foldLeft operation into another function, transformValuesByNestingResources (in 6e96460). But even after doing that, splitMainResourcesAndValueRdfData is still 273 lines long. :(

It took me really long time to understand what the splitMainResourcesAndValueRdfData does, but at least now with transformValuesByNestingResources it is a tiny bit easier to read.

nestedResource = Some(dependentResource)
)
} else if (dependentResourceIrisNotVisible.contains(dependentResourceIri)) {

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Apr 6, 2020
Contributor

then this means that previously there was no difference between the deleted dependant resource and forbidden one, in both cases link value was returned, but now if the dependant resource is forbidden (i.e. user does not have permission to see), then it is skipped. If the dependant resource is deleted, the link value is however returned. Have I understood it right?
If so, when a user requests a resource, the link value property itself will not be visible to the user either if the user does not have permission to see the dependent resource, right?

This comment has been minimized.

@benjamingeer

benjamingeer Apr 7, 2020
Author Collaborator

now if the dependant resource is forbidden (i.e. user does not have permission to see), then it is skipped. If the dependant resource is deleted, the link value is however returned. Have I understood it right?

Yes. Do you think this makes sense? Or should we do the same thing in both cases?

If so, when a user requests a resource, the link value property itself will not be visible to the user either if the user does not have permission to see the dependent resource, right?

Right, it should not be visible. I will check and see if that's what really happens.

This comment has been minimized.

@benjamingeer

benjamingeer Apr 7, 2020
Author Collaborator

It turns out that the link value property was not filtered out if we filtered out all its values. I've fixed this and added a test in 35fafe4. Good catch, thanks!

This comment has been minimized.

@benjamingeer

benjamingeer Apr 7, 2020
Author Collaborator

We have to return a LinkValue that points to a deleted resource, so the user can change the link to point to a different resource. In the case of a LinkValue that points to a resource that the user doesn't have permission to see, I filtered it out because @loicjaouen said he preferred it that way. I think it makes sense because there isn't anything that the user could do with that link.

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Apr 7, 2020
Contributor

Yes. Do you think this makes sense? Or should we do the same thing in both cases?

yes, I believe it makes sense as it is now. In this way, the difference between a deleted dependant resource and a forbidden one is clear and the user will not be confused by seeing a link value but not its dependant resource.

This comment has been minimized.

@benjamingeer

benjamingeer Apr 7, 2020
Author Collaborator

OK great. In b990663 I've added a test that gets a link to a deleted resource, to check that the link value is returned correctly.

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Apr 7, 2020
Contributor

Looks good to me!

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Apr 6, 2020

@SepidehAlassi

I suggest that, in another branch/PR, you either remove these tests or replace them with tests to check the compatibility of releases, not the compatibility of commits, as you suggested yourself.

I actually have no idea what these tests do, how they work, or even where the code is that runs them. @tobiasschweizer and @subotic set this up and I don't know if it's documented anywhere. I could probably figure out how to turn them off, but maybe @tobiasschweizer and @subotic would be better able to deal with this properly.

After that PR is merged to the develop branch, with a control merge, the problem of failing tests of this PR must be solved.

Does this mean I can't merge this PR until someone writes a whole new testing framework for compatibility between Knora and knora-api-js-lib? That sounds like it could take a long time.

@tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Apr 7, 2020

@SepidehAlassi

I suggest that, in another branch/PR, you either remove these tests or replace them with tests to check the compatibility of releases, not the compatibility of commits, as you suggested yourself.

I actually have no idea what these tests do, how they work, or even where the code is that runs them. @tobiasschweizer and @subotic set this up and I don't know if it's documented anywhere. I could probably figure out how to turn them off, but maybe @tobiasschweizer and @subotic would be better able to deal with this properly.

After that PR is merged to the develop branch, with a control merge, the problem of failing tests of this PR must be solved.

Does this mean I can't merge this PR until someone writes a whole new testing framework for compatibility between Knora and knora-api-js-lib? That sounds like it could take a long time.

I think we should have a look at this today. We could decide on a good time at the end of the Scrum meeting.

@SepidehAlassi
Copy link
Contributor

@SepidehAlassi SepidehAlassi commented Apr 7, 2020

@benjamingeer

Does this mean I can't merge this PR until someone writes a whole new testing framework for compatibility between Knora and knora-api-js-lib? That sounds like it could take a long time.

No, what I mean is that: Since the problems with tests should be resolved anyway before the merge, I suggest you solve the problem in another branch that can be reviewed fast, (even if solving the test problems means removing the tests) then merge it to develop first, then merge the develop branch into this branch.

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Apr 7, 2020

I suggest you solve the problem in another branch that can be reviewed fast, (even if solving the test problems means removing the tests) then merge it to develop first, then merge the develop branch in this branch.

OK will do.

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Apr 7, 2020

@tobiasschweizer

I think we should have a look at this today. We could decide on a good time at the end of the Scrum meeting.

OK great, thanks.

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Apr 7, 2020

@tobiasschweizer You asked for a copy-and-paste of the failing JS lib tests in this PR:

**************************************************
*                    Failures                    *
**************************************************

1) workspace-project App perform a fulltext search
  - Expected '15' to equal '21'.

2) workspace-project App perform a fulltext search count query
  - Expected '15' to equal '21'.

3) workspace-project App perform a label search
  - Expected '15' to equal '21'.

4) workspace-project App perform an extended search
  - Expected '23' to equal '25'.
@tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Apr 7, 2020

@tobiasschweizer You asked for a copy-and-paste of the failing JS lib tests in this PR:

**************************************************
*                    Failures                    *
**************************************************

1) workspace-project App perform a fulltext search
  - Expected '15' to equal '21'.

2) workspace-project App perform a fulltext search count query
  - Expected '15' to equal '21'.

3) workspace-project App perform a label search
  - Expected '15' to equal '21'.

4) workspace-project App perform an extended search
  - Expected '23' to equal '25'.

Ok, this is because knora-api returns less results now. I can easily fix this in a separate knora-api-js-lib PR, so then the tests pass again in both repos. In the meantime please go ahead as agreed.

edit: I think I don't have to check for a certain number of results at all, just have to check that there is a number returned.

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Apr 7, 2020

@SepidehAlassi Is this PR OK to merge now? If so, please don't forget to click the "approve" button.

@SepidehAlassi
Copy link
Contributor

@SepidehAlassi SepidehAlassi commented Apr 7, 2020

@SepidehAlassi Is this PR OK to merge now? If so, please don't forget to click the "approve" button.

yes, I believe it is. :-)

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Apr 7, 2020

@SepidehAlassi Many thanks for the review!

@benjamingeer benjamingeer merged commit 992596e into develop Apr 7, 2020
7 of 8 checks passed
7 of 8 checks passed
Build Everything
Details
API Unit Tests
Details
API E2E Tests
Details
API Integration Tests
Details
JS Lib Tests (11.x) JS Lib Tests (11.x)
Details
Upgrade Unit Tests
Details
Upgrade Integration Tests
Details
Docs Build Test
Details
@benjamingeer benjamingeer deleted the wip/1543-forbidden-resource branch Apr 7, 2020
@benjamingeer benjamingeer mentioned this pull request May 7, 2020
10 of 10 tasks complete
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.

4 participants