Checks on resourceid (Public Links) - #2494
Conversation
|
not entirely sure of the implications of removing that equality statement, should instead be checked whether the id is null? Bottom line is this guard is assuming id's are integers, where they could be strings now (depending on the back end) so perhaps this is just a band-aid on a wider issue |
|
@refs can you instead remove the There is a slight risk of breakage as I noticed that in many places in Phoenix some values are not consistently converted (or not converted) to integers. CI will tell. |
|
also note: the code in question does not refer to the share ids but to the id of the source that is shared, here a file id. The actual share id is in another attribute, probably |
Ah, yes, I meant resource id. Getting used to the domain terminology 💃 |
|
|
|
@PVince81 I realized the initial state of the PR completely breaks the public share listing later on. In order to support both oc10 + ocis that filter should work both on autoincrement ids (oc10) and base64[1] resourceid[2]. Whether this works with oc10 we'll see on CI (haven't tested locally, only reva). |
refs
changed the title
|
Hmm, seems CI is green. so this likely means that we're lucky and both file ids and share ids are strings already in all places. |
| }, | ||
| compareIds (x, y) { | ||
| if (!isNaN(x)) { // OC10 autoincrement id | ||
| return parseInt(x) === parseInt(y) |
There was a problem hiding this comment.
parseInt(x, 10) === parseInt(y, 10). without this leading zeroes would cause Javascript to parse numbers as octal...
There was a problem hiding this comment.
Ah, I was was aware of the radix problem but was blatantly relying on ES5 definition:
If radix is undefined or 0, it is assumed to be 10 except when the number begins with the character pairs 0x or 0X, in which case a radix of 16 is assumed. If radix is 16, the number may also optionally begin with the character pairs 0x or 0X.
the definition applies also on Number ES6:
This method has the same functionality as the global parseInt() function:
Number.parseInt === parseInt; // true
and is part of ECMAScript 2015 (its purpose is modularization of globals). Please see parseInt() for more detail and examples.
In any case, a check for hex would make it less error prone, but octal should not be a concern, unless someone's using a version older than ES5... .
In any case, to make things clear I'd add the radix 💃
| compareIds (x, y) { | ||
| if (!isNaN(x)) { // OC10 autoincrement id | ||
| return parseInt(x) === parseInt(y) | ||
| } else if (atob(y).split(':').length === 2) { // OC10: https://github.com/cs3org/reva/blob/e7830e2f752a05a081178ed26c384ced983b8fde/internal/http/services/owncloud/ocdav/ocdav.go#L169-L175 |
There was a problem hiding this comment.
if this is something we'll do more in the future for extracting the id, maybe we need a separate function for just extraction ?
something like {x, y, z, ...} = extractCS3IdParts(id) (not sure how many parts it has)
There was a problem hiding this comment.
now thinking, maybe this should me done on SDK level. The SDK is here to abstract away whether Webdav, OCS or CS3 is being used to avoid having API-specific logic in the frontend code.
@DeepDiver1975 thoughts ?
There was a problem hiding this comment.
I'm not entirely sure this is going to stay this way, perhaps @butonic can shed a bit more light, as the only place I found this requirement is alone in the reva code
There was a problem hiding this comment.
@butonic any comments here ?
if we don't know let's add a "TODO: move to common location or library" then
|
bump, before this gets stale. Is this safe to merge or requires more deliberation? |
|
@refs I suggest to move the id splitting logic to a helper function, maybe call it "splitOcisId" or "splitCs3Id" just to make sure future developers won't scratch their head about why there are two code parts. Make it return Then we can ship this and discuss later whether to keep this method or not. |
3 participants
Description
Gets rid of share id type checking
Motivation and Context
Reva id's are strings, therefore this guard is preventing them from being rendered.
How Has This Been Tested?
✋
Types of changes