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

cache displaynames in ocs #1161

Merged
merged 2 commits into from
Sep 22, 2020
Merged

Conversation

butonic
Copy link
Contributor

@butonic butonic commented Sep 11, 2020

The ocs list shares endpoint may need to fetch the displayname for multiple different users. We are now caching the lookup fo 60 seconds to save redundant RPCs to the users service.

  • get a better cache implementation? moving to a pkg because caching a username for 60sec is good enough for now.

  • use a displayname cache or better a full blown user struct cache that could bu used in other handlers as well? for now, the sharing code really only needs to look up displaynames

  • implement a recent users (and groups) cache whenever we get a new userid in the access token so we always have the most recent users cached? left for a subsequent PR. It would need to make the cache part of the handler that is used in all ocs handlers.

@update-docs
Copy link

update-docs bot commented Sep 11, 2020

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@butonic
Copy link
Contributor Author

butonic commented Sep 18, 2020

@ishank011 @labkode this kind of caching should at least happen per request. other services could use it as well, eg. the strage provider if it needs to look up the username of a user when it only has the id.

should we have a reusable type of cache or just get sth like this in asap.

@butonic
Copy link
Contributor Author

butonic commented Sep 21, 2020

I propose to get this in as is. It makes listing shares usable. We can improve further if needed.

@ishank011 @labkode review and merge?

@butonic butonic marked this pull request as ready for review September 21, 2020 12:08
labkode
labkode previously approved these changes Sep 21, 2020
@phil-davis
Copy link
Contributor

https://cloud.drone.io/cs3org/reva/2609/6/7

  Background:                                                                    # /drone/src/tests/acceptance/features/apiOcisSpecific/apiSharePublicLink2-updatePublicLinkShare.feature:4
    Given using OCS API version "1"                                              # FeatureContext::usingOcsApiVersion()
    And user "Alice" has been created with default attributes and skeleton files # FeatureContext::userHasBeenCreatedWithDefaultAttributes()

  @issue-ocis-reva-243 @issue-ocis-reva-349 @issue-37653
  Scenario Outline: API responds with a full set of parameters when owner changes the expireDate of a public share # /drone/src/tests/acceptance/features/apiOcisSpecific/apiSharePublicLink2-updatePublicLinkShare.feature:10
    Given using OCS API version "<ocs_api_version>"                                                                # FeatureContext::usingOcsApiVersion()
    When user "Alice" creates a public link share using the sharing API with settings                              # FeatureContext::userCreatesAPublicLinkShareWithSettings()
      | path | FOLDER |
    And user "Alice" updates the last share using the sharing API with                                             # FeatureContext::userUpdatesTheLastShareWith()
      | expireDate | +3 days |
    Then the OCS status code should be "<ocs_status_code>"                                                         # OCSContext::theOCSStatusCodeShouldBe()
    And the OCS status message should be "OK"                                                                      # OCSContext::theOCSStatusMessageShouldBe()
    And the HTTP status code should be "200"                                                                       # FeatureContext::thenTheHTTPStatusCodeShouldBe()
    And the fields of the last response to user "Alice" should include                                             # FeatureContext::checkFields()
      | id                         | A_STRING             |
      | share_type                 | public_link          |
      | uid_owner                  | %username%           |
      | displayname_owner          | %displayname%        |
      | permissions                | read                 |
      | stime                      | A_NUMBER             |
      | parent                     |                      |
      | expiration                 | A_STRING             |
      | token                      | A_STRING             |
      | uid_file_owner             | %username%           |
      | displayname_file_owner     | %displayname%        |
      | additional_info_owner      |                      |
      | additional_info_file_owner |                      |
      | state                      | 0                    |
      | item_type                  | folder               |
      | item_source                | A_STRING             |
      | path                       | /FOLDER              |
      | mimetype                   | httpd/unix-directory |
      | storage_id                 | A_STRING             |
      | storage                    | A_NUMBER             |
      | file_source                | A_STRING             |
      | file_target                | /FOLDER              |
      | mail_send                  | 0                    |
      | name                       |                      |

    Examples:
      | ocs_api_version | ocs_status_code |
      | 1               | 100             |
        │ displayname_owner has unexpected value ''
        │ 
        displayname_owner doesn't have value 'Alice Hansen'
        Failed asserting that false is true.
      | 2               | 200             |
        │ displayname_owner has unexpected value ''
        │ 
        displayname_owner doesn't have value 'Alice Hansen'
        Failed asserting that false is true.

displayname_owner has gone missing in a response.

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@butonic
Copy link
Contributor Author

butonic commented Sep 22, 2020

rebased ... the feature lines seemed to be off ... 🤞

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@butonic
Copy link
Contributor Author

butonic commented Sep 22, 2020

@labkode fixed tests, merge if you please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants