Use fileid as string in web UI #30435

PVince81 · 2018-02-09T09:36:18Z

Description

Because of limitations to JS max int, we now use strings instead of
integers.

Related Issue

Fixes #30434

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

PVince81 · 2018-02-09T10:10:17Z

TODO: fix sharing "item_source" and "file_source"
TODO: federated share "file_id" field

PVince81 · 2018-02-09T10:12:28Z

versions file id

PVince81 · 2018-02-09T10:12:38Z

comments object id...

PVince81 · 2018-02-09T10:13:43Z

activity file id...

PVince81 · 2018-02-09T10:31:47Z

previews in activity seem broken

PVince81 · 2018-02-09T10:37:39Z

gallery slideshow broken...

codecov · 2018-02-09T10:40:48Z

Codecov Report

Merging #30435 into master will not change coverage.
The diff coverage is 75%.

@@            Coverage Diff            @@
##             master   #30435   +/-   ##
=========================================
  Coverage     61.59%   61.59%           
  Complexity    18505    18505           
=========================================
  Files          1090     1090           
  Lines         61108    61108           
=========================================
  Hits          37642    37642           
  Misses        23466    23466

Impacted Files	Coverage Δ	Complexity Δ
apps/files_sharing/lib/API/Remote.php	`0% <0%> (ø)`	`12 <0> (ø)`	⬇️
apps/files_sharing/lib/API/Share20OCS.php	`91.64% <100%> (ø)`	`152 <0> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b420d6...48140f1. Read the comment docs.

PVince81 · 2018-02-09T10:54:04Z

seems previews and gallery already broken on master, so likely unrelated to this change

PVince81 · 2018-02-09T10:56:55Z

I think I've nailed most of the cases.

phil-davis · 2018-02-09T10:57:56Z

First run of drone had UI test fails at:

sharingInternalGroupsUsers
    And the sharing permissions of "User One" for "simple-folder" are set to # SharingContext::theSharingPermissionsOfAreSetTo()
| delete | no |

sharingExternalOther
    And the sharing permissions of "user1@%remote_server% (federated)" for "simple-folder" are set to # SharingContext::theSharingPermissionsOfAreSetTo()
| delete | no |

restrictSharing
    And the sharing permissions of "User One" for "simple-folder" are set to # SharingContext::theSharingPermissionsOfAreSetTo()
| share | no |

these are the only 3 places that the step the sharing permissions of "user" for "folder" are set to is used.

So I suspect that there is some issue with the sharing permissions checkboxes. I don't see immediately in the UI test code where it tries to traverse any HTML elements... that contain the file id in their xpath.

Let's see if it fails the same all the time...

Edit: the latest pushes fixed it - last 2 drone runs pass.

PVince81 · 2018-02-12T17:00:22Z

@phil-davis seems those problems disappeared?

phil-davis · 2018-02-12T17:02:56Z

@PVince81 yes, after you pushed more changes it was passing, so I don't know if the first fails were just unfortunate (it seemed a big coincidence that it was sharing test steps that failed in the 3 sharing UI test suites).

PVince81 · 2018-02-12T17:08:02Z

pinging more people to review the JS changes

VicDeo

The only question is about the way of casting to string

VicDeo · 2018-02-12T18:01:20Z

apps/files_sharing/lib/API/Remote.php

@@ -134,7 +134,7 @@ private static function extendShareInfo($share) {
 		$share['mtime'] = $info->getMtime();
 		$share['permissions'] = $info->getPermissions();
 		$share['type'] = $info->getType();
-		$share['file_id'] = $info->getId();
+		$share['file_id'] = '' . $info->getId();


http://php.net/manual/en/function.strval.php ?

PVince81 · 2018-02-15T11:11:24Z

anything else or does the code look ok ?

VicDeo

@PVince81 I'd vote against hardcore casting by concatenation.
there are more clean ways to get string in PHP: strval(5) or (string) 5

But it's up to you.
No other objections.

PVince81 · 2018-02-16T08:53:46Z

@VicDeo ok, since you insisted I'll replace "hardcore casting" 😄

PVince81 · 2018-02-16T08:56:30Z

replaced 4 occurrences to strval in PHP code and rebased + squashed

PVince81 · 2018-02-16T16:29:27Z

why is this upload test failing ? it seems to not find the lorem file

phil-davis · 2018-02-16T16:36:21Z

It is usually related to a lock left somewhere - I think the upload-overwrite fails because the file is already locked, then the content is checked and of course is the old content, not the expected new content.

If you get unlucky, this happens to you. This is the one I notice happening the most.

phil-davis · 2018-02-16T16:38:15Z

Go to the owncloud-log step and search for locked - if that can be fixed then other troubles will go away.

phil-davis · 2018-02-16T16:39:25Z

{"reqId":"tw5k1PGffGkswoDsxK13","level":0,"time":"2018-02-16T10:27:06+00:00","remoteAddr":"172.18.0.3","user":"user1","app":"core","method":"GET","url":"\/remote.php\/dav\/files\/user1\/simple-folder\/lorem.txt?c=1478997b23797c954f7ee8ea250c4c99&x=32&y=32&forceIcon=0&preview=1","message":"Generating preview for \"\/user1\/files\/simple-folder\/lorem.txt\" with \"OC\\Preview\\TXT\""}
{"reqId":"evWmtS6qlZ3OLkHVeCOM","level":4,"time":"2018-02-16T10:27:07+00:00","remoteAddr":"172.18.0.3","user":"user1","app":"webdav","method":"PUT","url":"\/remote.php\/dav\/files\/user1\/simple-folder\/lorem.txt","message":"Exception: {\"Message\":\"HTTP\\\/1.1 423 \\\"simple-folder\\\/lorem.txt\\\" is locked\",\"Exception\":\"OCA\\\\DAV\\\\Connector\\\\Sabre\\\\Exception\\\\FileLocked\",\"Code\":0,\"Trace\":\"#0 \\\/drone\\\/src\\\/lib\\\/public\\\/Events\\\/EventEmitterTrait.php(50): OCA\\\\DAV\\\\Connector\\\\Sabre\\\\File->OCA\\\\DAV\\\\Connector\\\\Sabre\\\\{closure}()\\n#1 \\\/drone\\\/src\\\/apps\\\/dav\\\/lib\\\/Connector\\\/Sabre\\\/File.php(258): OCA\\\\DAV\\\\Connector\\\\Sabre\\\\File->emittingCall(Object(Closure), Array, 'file', 'create')\\n#2 \\\/drone\\\/src\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(1129): OCA\\\\DAV\\\\Connector\\\\Sabre\\\\File->put(Resource id #53)\\n#3 \\\/drone\\\/src\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/CorePlugin.php(513): Sabre\\\\DAV\\\\Server->updateFile('files\\\/user1\\\/sim...', Resource id #53, NULL)\\n#4 [internal function]: Sabre\\\\DAV\\\\CorePlugin->httpPut(Object(Sabre\\\\HTTP\\\\Request), Object(Sabre\\\\HTTP\\\\Response))\\n#5 \\\/drone\\\/src\\\/lib\\\/composer\\\/sabre\\\/event\\\/lib\\\/EventEmitterTrait.php(105): call_user_func_array(Array, Array)\\n#6 \\\/drone\\\/src\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(479): Sabre\\\\Event\\\\EventEmitter->emit('method:PUT', Array)\\n#7 \\\/drone\\\/src\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(254): Sabre\\\\DAV\\\\Server->invokeMethod(Object(Sabre\\\\HTTP\\\\Request), Object(Sabre\\\\HTTP\\\\Response))\\n#8 \\\/drone\\\/src\\\/apps\\\/dav\\\/lib\\\/Server.php(279): Sabre\\\\DAV\\\\Server->exec()\\n#9 \\\/drone\\\/src\\\/apps\\\/dav\\\/appinfo\\\/v2\\\/remote.php(31): OCA\\\\DAV\\\\Server->exec()\\n#10 \\\/drone\\\/src\\\/remote.php(175): require_once('\\\/drone\\\/src\\\/apps...')\\n#11 {main}\",\"File\":\"\\\/drone\\\/src\\\/apps\\\/dav\\\/lib\\\/Connector\\\/Sabre\\\/File.php\",\"Line\":206,\"User\":\"user1\"}"}

suspiciously related to a "generating preview" message above it.

PVince81 · 2018-02-16T16:44:22Z

raised here: #30506

PVince81 · 2018-02-19T10:48:50Z

trying my luck again at the CI roulette...

phil-davis · 2018-02-19T10:54:55Z

@PVince81 you will win if you rebase

Because of limitations to JS max int, we now use strings instead of integers.

PVince81 · 2018-02-19T11:00:50Z

@ownclouders rebase

ownclouders · 2018-02-19T11:00:52Z

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

ownclouders · 2018-02-19T11:01:13Z

Automated rebase with GitMate.io was successful! 🎉

phil-davis · 2018-02-19T12:43:45Z

You are so lucky
https://drone.owncloud.com/owncloud/core/4034/137

+ git init
Initialized empty Git repository in /drone/src/.git/
+ git remote add origin https://github.com/owncloud/core.git
+ git fetch --no-tags --depth=50 origin +refs/pull/30435/merge:
fatal: The remote end hung up unexpectedly
exit status 128

I restarted drone

PVince81 · 2018-02-19T14:39:51Z

thanks for the luck. maybe someone should start a service where people can pool their luck in a single location for greater luck...

phil-davis · 2018-02-19T14:46:50Z

@PVince81 Is this to be backported so that these huge file ids can work in a 10.0.* release?

PVince81 · 2018-02-19T15:03:36Z

This is a bit dangerous, so no

labkode · 2018-05-31T13:06:15Z

@PVince81 We will need to have this functionality for our service, do you expect to to release for 10.1.*?

PVince81 · 2018-06-01T14:01:40Z

This will be in the next major release, yes. But not the minor one due to bigger risks.

I'm not sure whether the current automated tests already cover for all the cases touched by this PR.
If not, it would require to schedule a regression test of the complete files app and sharing UI.

lock · 2019-07-30T11:02:38Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

PVince81 added the 2 - Developing label Feb 9, 2018

PVince81 added this to the development milestone Feb 9, 2018

PVince81 self-assigned this Feb 9, 2018

PVince81 requested a review from DeepDiver1975 February 9, 2018 09:36

ownclouders added Type:Bug server labels Feb 9, 2018

PVince81 mentioned this pull request Feb 9, 2018

Devil in the details: File ID javascript limit #30434

Closed

PVince81 added 3 - To Review and removed 2 - Developing labels Feb 9, 2018

PVince81 requested review from VicDeo and felixheidecke February 12, 2018 17:08

VicDeo reviewed Feb 12, 2018

View reviewed changes

VicDeo approved these changes Feb 15, 2018

View reviewed changes

PVince81 force-pushed the js-fileid-as-string branch from 591a38f to e255991 Compare February 16, 2018 08:56

Vincent Petry added 3 commits February 19, 2018 11:00

Use fileid as string in files app

149d4e8

Because of limitations to JS max int, we now use strings instead of integers.

Don't convert file id to int in FileInfoModel

953e7ca

File id as string for sharing and other components

48140f1

ownclouders force-pushed the js-fileid-as-string branch from e255991 to 48140f1 Compare February 19, 2018 11:01

PVince81 merged commit 731c3d8 into master Feb 19, 2018

PVince81 deleted the js-fileid-as-string branch February 19, 2018 14:39

ownclouders added the status/STALE label Jul 19, 2018

lock bot locked as resolved and limited conversation to collaborators Jul 30, 2019

Use fileid as string in web UI #30435

Use fileid as string in web UI #30435

Conversation

PVince81 commented Feb 9, 2018

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

PVince81 commented Feb 9, 2018 • edited Loading

PVince81 commented Feb 9, 2018 • edited Loading

PVince81 commented Feb 9, 2018 • edited Loading

PVince81 commented Feb 9, 2018 • edited Loading

PVince81 commented Feb 9, 2018

PVince81 commented Feb 9, 2018

codecov bot commented Feb 9, 2018 • edited Loading

Codecov Report

PVince81 commented Feb 9, 2018

PVince81 commented Feb 9, 2018

phil-davis commented Feb 9, 2018 • edited Loading

PVince81 commented Feb 12, 2018

phil-davis commented Feb 12, 2018

PVince81 commented Feb 12, 2018

VicDeo left a comment

Choose a reason for hiding this comment

VicDeo Feb 12, 2018

Choose a reason for hiding this comment

PVince81 Feb 13, 2018

Choose a reason for hiding this comment

PVince81 commented Feb 15, 2018

VicDeo left a comment • edited Loading

Choose a reason for hiding this comment

PVince81 commented Feb 16, 2018

PVince81 commented Feb 16, 2018

PVince81 commented Feb 16, 2018

phil-davis commented Feb 16, 2018

phil-davis commented Feb 16, 2018

phil-davis commented Feb 16, 2018

PVince81 commented Feb 16, 2018

PVince81 commented Feb 19, 2018

phil-davis commented Feb 19, 2018

PVince81 commented Feb 19, 2018

ownclouders commented Feb 19, 2018

ownclouders commented Feb 19, 2018

phil-davis commented Feb 19, 2018

PVince81 commented Feb 19, 2018

phil-davis commented Feb 19, 2018

PVince81 commented Feb 19, 2018

labkode commented May 31, 2018

PVince81 commented Jun 1, 2018

lock bot commented Jul 30, 2019

PVince81 commented Feb 9, 2018 •

edited

Loading

PVince81 commented Feb 9, 2018 •

edited

Loading

PVince81 commented Feb 9, 2018 •

edited

Loading

PVince81 commented Feb 9, 2018 •

edited

Loading

codecov bot commented Feb 9, 2018 •

edited

Loading

phil-davis commented Feb 9, 2018 •

edited

Loading

VicDeo left a comment •

edited

Loading