Properly decode id from URI

[nickvergessen]

When parsing an href to get the id of a WebdavNode, make sure to also
decode it.

Downstream 27511

[mention-bot]

@nickvergessen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @LukasReschke to be a potential reviewer.

[MorrisJobke]

JS unit tests are failing 🙈

[nickvergessen]

Well feel free to fix, I have no idea about the code.

[MorrisJobke]

@danxuliu Do you want to look into this maybe? 😉

[danxuliu]

@danxuliu Do you want to look into this maybe? 😉

The tests fail because the commit depends on another commit that is not included yet in Nextcloud.

However, some functional changes seem to be included in that commit (like using PUT instead of POST if the model to create already contains an id); I am not familiar enough (or at all :P ) with WebDAV to know whether those functional changes are safe or not, so I have not cherry-picked the commit into this pull request yet. I will try to investigate it, though ;)

Besides, between the two commits there is another one that changes how requests are sent to the server when using MKCOL; it does not affect the changes backported here, but it may be good to include it nevertheless to keep the file in better sync with the ownCloud version (although I do not know if that should be done in this same pull request or a different one).

[codecov]

Codecov Report

Merging #4528 into master will decrease coverage by 0.12%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #4528      +/-   ##
============================================
- Coverage     54.14%   54.02%   -0.13%     
+ Complexity    22353    21858     -495     
============================================
  Files          1379     1283      -96     
  Lines         85559    76293    -9266     
  Branches       1329        0    -1329     
============================================
- Hits          46328    41215    -5113     
+ Misses        39231    35078    -4153

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Diagnostics/QueryLogger.php	0% <0%> (-100%)	6% <0%> (-3%)
lib/private/Diagnostics/EventLogger.php	0% <0%> (-100%)	5% <0%> (-4%)
lib/private/Diagnostics/Event.php	0% <0%> (-55.56%)	8% <0%> (ø)
lib/private/Diagnostics/Query.php	0% <0%> (-38.47%)	7% <0%> (+1%)
apps/dav/lib/DAV/Sharing/Backend.php	71.08% <0%> (-16.87%)	17% <0%> (ø)
lib/private/legacy/db/statementwrapper.php	66.66% <0%> (-12.5%)	10% <0%> (+1%)
lib/private/App/CodeChecker/InfoChecker.php	69.56% <0%> (-9.16%)	24% <0%> (+9%)
apps/theming/lib/Capabilities.php	71.42% <0%> (-6.35%)	2% <0%> (-1%)
lib/private/legacy/template/functions.php	6.89% <0%> (-6.32%)	0% <0%> (ø)
lib/private/Cache/CappedMemoryCache.php	93.75% <0%> (-6.25%)	15% <0%> (ø)
... and 270 more

[danxuliu]

Here are my conclusions after reviewing the changes introduced in the commit Add Webdav specific models and collections:

• convertModelAttributesToDavProperties now ignores those attributes that are not mapped in the given davProperties object instead of passing them anyway to the server; in the end, that object comes either from the davProperties property of the model passed to davSync, or from the davProperties property of the options parameter passed to davSync or davCall. In any case, the davProperties properties defined in the server repository (in apps/comments/js/comment*(collection|model).js and core/js/systemtags/systemtagmodel.js) map all the "official" attributes defined for those models, so this change should make no difference.

• davSync behaviour remains virtually unchanged for those models using it in the server repository. Before, it issued a MKCOL request when the model had the hasInnerCollection property and had to be updated; however, that property was not used anywhere in the server repository. Now, the MKCOL request is issued only when creating a WebdavCollectionNode model, which was introduced in the commit itself, so it is not used anywhere.

  The only relevant difference is that, before, POST was issued when creating a model, and now POST is issued only when creating a model that has no id yet; if it has an id PUT is issued instead. However, when saving a model, Backbone uses a create method only if isNew returns true; the default behaviour of isNew is to return true when there is no id attribute so, unless isNew is redefined (like in the new models introduced in the commit), creating a model through davSync will issue a POST request. As no model using davSync in the server repository redefines isNew the behaviour remains unchanged.

• the extra parameter added to callPropPatch does not change anything when not set, and it is only set when issuing a MKCOL request (the change sets all the model attributes when issuing the PROPPATCH that follows the MKCOL request; I am missing something, because I must admit that I do not get why it was not done before... but anyway, that is not relevant to the current behaviour). The MKCOL request is not issued from davSync as explained before, and neither from a direct call to davCall because it is not called anywhere in the server repository (apart from davSync).

In short, for the code using oc-backbone-webdav.js in the server repository the changes should make no difference, but I can not assure that they make no difference for other applications using it; it depends on whether they mapped all their model attributes to DAV properties (which they should be doing already anyway) or not, on whether they used the hasInnerCollection property or not (I would be surprised if it was used anywhere, though, as it was a rather obscure setting), on whether it called davCall directly or not (but probably they will be using davSyncinstead), and on whether it redefined isNew to not depend on the id attribute or not.

With all that, I have modified this pull request to include the commit; it seems safe to add it, it is necessary for the unit tests included in the original fix and if any app breaks after merging this pull request... well, we can fix the app anyway ;)

However, I have not added the intermediate commit as it is not related to this issue (it changes how requests are made; instead of sending an empty MKCOL and then updating it with a PROPPATCH a single MKCOL request that includes the properties is sent), but I can do it if you want.

[MorrisJobke]

@danxuliu Should we try to get this in 13 or is 14 fine?

[danxuliu]

@MorrisJobke I have no preference. On one hand, it is a bug fix, so it would be good to get this in. On the other hand, I am not aware of any place were the bug appears, and it may break apps that use oc-backbone-webdav.js (although it is unlikely). So... as you wish :-)

[nickvergessen]

Obsoleted by #7765