johnnyhowey/liferay-portal — pull request #76 by lipusz (2014-06-27 09:37:14+00:00)
Title: [TECHNICAL-SUPPORT | July 8]  LPS-48046 Opening a document via WebDAV doesn't increase the readCount (caused by LPS-29678)

https://issues.liferay.com/browse/LPS-48046

Hey Jonathan,

I'm sending this pull to you because you were the author of https://issues.liferay.com/browse/LPS-29678 that changed the behaviour. So, I'd like to let you know to do a double check before we forwarding it to Sergio.

=== Technical details
1- Prior to LPS-29678 the method getContentAsStream() looked like this:

````java
public class DLFileEntryResourceImpl(6012) ... {
//...
@Override
public InputStream getContentAsStream() throws WebDAVException {
try {
String version = StringPool.BLANK;

if (PropsValues.DL_WEBDAV_HOLD_LOCK) {

// Get last version regardless of status

DLFileVersion fileVersion = _fileEntry.getLatestFileVersion();

version = fileVersion.getVersion();
}

return DLFileEntryLocalServiceUtil.getFileAsStream(
_webDavRequest.getCompanyId(), _webDavRequest.getUserId(),
_fileEntry.getGroupId(), _fileEntry.getFolderId(),
_fileEntry.getName(), version);
}
catch (Exception e) {
throw new WebDAVException(e);
}
}
//...
}
````
and in 6120,

````java
public class DLFileEntryResourceImpl(6120) ... {
//...
@Override
public InputStream getContentAsStream() throws WebDAVException {
try {
String version = StringPool.BLANK;

return _fileEntry.getContentStream(version);
}
catch (Exception e) {
throw new WebDAVException(e);
}
}
//...
}
````

2- Then, in LPS-29678 you did this change: liferay@6d3a3530cad8353734c07fac280e66f4285b338e

Why does it matter? The readCount got incremented in the old versions when properties
````
asset.entry.increment.view.counter.enabled
````
and
````
dl.file.entry.read.count.enabled
````
were set to "true", because

in 6012, the following service call
````java
DLFileEntryLocalServiceUtil.getFileAsStream(
_webDavRequest.getCompanyId(), _webDavRequest.getUserId(),
_fileEntry.getGroupId(), _fileEntry.getFolderId(),
_fileEntry.getName(), version);
````
went to

````java
public class DLFileEntryLocalServiceImpl(6012) ... {
//...
public InputStream getFileAsStream(
long companyId, long userId, long groupId, long folderId,
String name, String version)
throws PortalException, SystemException {

DLFileEntry fileEntry = dlFileEntryPersistence.findByG_F_N(
groupId, folderId, name);

if (userId > 0) {
dlFileRankLocalService.updateFileRank(
groupId, companyId, userId, fileEntry.getFileEntryId(),
new ServiceContext());
}

if (PropsValues.DL_FILE_ENTRY_READ_COUNT_ENABLED) {
fileEntry.setReadCount(fileEntry.getReadCount() + 1);

dlFileEntryPersistence.update(fileEntry, false);

assetEntryLocalService.incrementViewCounter(
userId, DLFileEntry.class.getName(),
fileEntry.getFileEntryId());

List fileShortcuts =
dlFileShortcutPersistence.findByToFileEntryId(
fileEntry.getFileEntryId());

for (DLFileShortcut fileShortcut : fileShortcuts) {
assetEntryLocalService.incrementViewCounter(
userId, DLFileShortcut.class.getName(),
fileShortcut.getFileShortcutId());
}
}

if (Validator.isNotNull(version)) {
return dlLocalService.getFileAsStream(
companyId, fileEntry.getRepositoryId(), name, version);
}
else {
return dlLocalService.getFileAsStream(
companyId, fileEntry.getRepositoryId(), name,
fileEntry.getVersion());
}
}
//...
}
````

in 6120, the following call
````java
_fileEntry.getContenAsStream(version);
````

ended-up in (e.g.)
````java
public class LiferayFileEntry(6120) ... {
//...
@Override
public InputStream getContentStream(String version) throws PortalException {
InputStream inputStream = _dlFileEntry.getContentStream(version);

try {
DLAppHelperLocalServiceUtil.getFileAsStream(
PrincipalThreadLocal.getUserId(), this, true);
}
catch (Exception e) {
_log.error(e);
}

return inputStream;
}
//...
}
````

3- Though, the new logic (since LPS-29678) calls getContentAsStream on the FileVersion with "false".

I think, opening a file via WebDAV is like if we downloaded the given file, so the "readCount" should be increased, if property (in master/7.0.x)

````
buffered.increment.enabled
````
is true and/or count is configured for DM.

If you agree with my change, in my opinion, it would be a good idea to refer to this explanation from the forwarded pull to make further reviewing easier.

Regards,
Tibor