I have quite a bunch of questions:

• Do those entries with null values have any purpose? Why are those entries being stored?
• If they don't have any purpose:
  • Where are those values coming from?
  • Why it isn't being enforced by the DB?

I can understand there could be entries with a fileid pointing to a missing file due to the lack of referencial integrity, but I don't see any use for the case where the fileid is null.

• Do those entries with null values have any purpose?

I am quite not sure if the null values have any purpose. While working on the CleanProperties, the insight I had is the fileid was pointing towards the fileid in the filecache. The whole purpose of this background job was to cleanup the traces present in the properties table which were refering to the fileid in filecache, and the filecache id was missing or say not there.

Why are those entries being stored?

Looking at the code flow, this is what I have deduced:

• Why is the entry stored with the null value -> https://github.com/owncloud/core/blob/master/apps/dav/lib/DAV/FileCustomPropertiesBackend.php#L186-L192. All it checks is if the property exists or not.
• Which part of the code provides the null value for the fileid -> https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/CacheEntry.php#L55 ( I have tested this by slightly modifying the code and the value NULL gets in to the fileid of properties table ).

• Where are those values coming from?
  I think this query is related with my response to query : Why are those entries being stored?

• Why it isn't being enforced by the DB?
  Not much of insight here.

• Backtrace and figure out why $node->getId(); gets NULL.

Again the code flow while tracing the $node->getId() is:

• https://github.com/owncloud/core/blob/master/apps/dav/lib/Connector/Sabre/Node.php#L234 ( call to getId)
• https://github.com/owncloud/core/blob/master/lib/private/Files/FileInfo.php#L139
• https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/CacheEntry.php#L52-L56

So as per the codeflow, the chance to get the NULL, is if the data is fileid offset is not present in the CacheEntry.

In the #36062 (comment) I have used the command below to create properties:
curl -k -D - -u admin:admin -X PROPPATCH -H "Content-Type: text/xml" --data-binary "@proppatch.xml" http://localhost/testing3/remote.php/webdav/a.txt

I have done an expirement by deleting the file before it reaches https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Cache.php#L129 and delete the file, which would make $data as false and hence it will throw exception https://github.com/owncloud/core/blob/master/apps/dav/lib/Connector/Sabre/ObjectTree.php#L193

<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\NotFound</s:exception>
  <s:message>File with name a.txt could not be located</s:message>
</d:error>

Now say even if I delete the file from oC after the SQL is executed https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/Cache.php#L129, then the data gets populated. And hence the fileid which was stored in the $data gets used in the properties table.

Humm, its little hard to pin point from where the data is not set and the code is getting propagated. Because to reach https://github.com/owncloud/core/blob/master/lib/private/Files/Cache/CacheEntry.php#L52-L56 and have the data not set for the file, then I am not sure how the exception is getting escaped.

Check with .part files. The other option is that the target node is the root node, but it should have the fileid.

If the cause is around https://github.com/owncloud/core/blob/master/apps/dav/lib/DAV/AbstractCustomPropertiesBackend.php#L207 , I think, the same way we're returning if the node is null, we could do the same if path refers to a .part file.

We might need to investigate how those properties end up there (while curl helps to investigate and reproduce the problem, no user is using curl).

The other thing to investigate is that, accordingly to the original ticket, the property set is {http://owncloud.org/ns}calendar-enabled which references to https://github.com/owncloud/core/blob/master/apps/dav/lib/CalDAV/Calendar.php#L180
This means that the issue might be related to the calendar app somehow.

This means that the issue might be related to the calendar app somehow.

I am currently looking into the calendar app. I will post the updates once I get some clue.

Looking at the code flow, if I provide url: remote.php/dav/calendars/admin/testing1 ( where admin is the user and testing1 is the uri of calendar, for calendar-enabled, the property gets updated/added to dav_propreties table, instead of properties table. This is the method https://github.com/owncloud/core/blob/master/apps/dav/lib/DAV/MiscCustomPropertiesBackend.php#L111 which updates/inserts the data to the table. Now I am bit worried how the user got the data updated in the properties table, with calendar-enabled. I will ping the user in #35407 to know more about the same.

Codecov Report

Merging #36062 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #36062   +/-   ##
=======================================
  Coverage   53.85%   53.85%           
=======================================
  Files          63       63           
  Lines        7377     7377           
  Branches     1301     1301           
=======================================
  Hits         3973     3973           
  Misses       3019     3019           
  Partials      385      385

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 95e7aa0...05e0f25. Read the comment docs.

The code looks fine, but I'm still reluctant to approve without knowing the cause of those null values reaching the DB.

If they're aren't allowed, the DB should forbid those values, and any code trying to set null values should be fixed (the code should remove the DB entry instead).
If they're allowed, maybe there is code relying on those null values. Then, the fix shouldn't be "remove the null values" but "null values aren't being handled properly"

Are you suggesting using a DB "NOT NULL" constraint instead of a background job ?
We'd then need to move the cleanup code into a migration then before doing the change.

Yes. If null values aren't allowed, the DB is the best place to enforce this constraint.
The problem is that, if there is code that is adding those values, that code will explode badly, and at the moment we're not sure where such code is or if it has been patched already.

@VicDeo I see a comment at https://github.com/owncloud/core/blob/master/apps/dav/appinfo/Migrations/Version20170116170538.php#L73-L74. Let me know if this is still applicable. A small context would be: there caused an issue with fileid accepting null values for oc_properties table, for a background job ( CleanProperties ). If you read the comments above, it would be ideal to fix it in the db. While I was looking through the migrations, I saw this comment. Let me know if its ok to change fileid to not null for oc_properties table.

@sharidas over two years passed, I barely can remember the context. Most likely adding a new column to the existing table produced null values in this column.

IIRC the migration process had 4 steps split over 3 migration files:

1. add a new nullable fileId column (Schema migration N1)
2. populate all existing entries with fileId (Sql migration N1)
3. drop all orphaned entries where isNull(fileId) === true (Sql migration N1)
4. drop user and property_path columns and make the fileId column not nullable. (Schema migration N2)

Which means that I made a mistake in the step 4. See the comment
https://github.com/owncloud/core/blob/master/apps/dav/appinfo/Migrations/Version20170202220512.php#L30

and the actual constraint https://github.com/owncloud/core/blob/master/apps/dav/appinfo/Migrations/Version20170202220512.php#L53

@sharidas please close in favor of the new PR and link it. I think @VicDeo's justification is enough.

Closing this PR in favour of #36084