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

Conversation

benjamingeer
Copy link

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

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

@daschbot
Copy link
Collaborator

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

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
@daschbot
Copy link
Collaborator

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

Choose a reason for hiding this comment

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

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.

docs/src/paradox/05-internals/design/api-v2/gravsearch.md Outdated Show resolved Hide resolved

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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. :(

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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!

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me!

@benjamingeer
Copy link
Author

@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

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

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
Author

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

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

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

@SepidehAlassi
Copy link
Contributor

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

@SepidehAlassi Many thanks for the review!

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.

Use ForbiddenResource consistently
4 participants