[GitHub] [jackrabbit-oak] mreutegg commented on a diff in pull request #863: OAK-10127 - Log warn message when MongoDB document is big

2023-05-05 Thread via GitHub 


mreutegg commented on code in PR #863:
URL: https://github.com/apache/jackrabbit-oak/pull/863#discussion_r1185993139


##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##
@@ -1470,8 +1487,18 @@ public  boolean create(Collection 
collection, List SIZE_LIMIT) {

Review Comment:
   How about removing this somewhat arbitrary threshold and simply log memory 
usage for each document? At this point we know some Bson size exceeded the 
limit. Readers of the log message can then interpret which document was most 
likely the culprit.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [jackrabbit-oak] mreutegg commented on a diff in pull request #863: OAK-10127 - Log warn message when MongoDB document is big

2023-05-05 Thread via GitHub 


mreutegg commented on code in PR #863:
URL: https://github.com/apache/jackrabbit-oak/pull/863#discussion_r1185989489


##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDBExceptionTest.java:
##
@@ -31,11 +31,13 @@
 import org.junit.BeforeClass;
 import org.junit.Test;
 
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
 import static java.util.Collections.singletonList;
 import static org.hamcrest.Matchers.containsString;
-import static org.junit.Assert.assertThat;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.junit.Assert.*;

Review Comment:
   Please don't import with a wildcard.



##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##
@@ -1470,6 +1487,19 @@ public  boolean create(Collection 
collection, List

[jira] [Assigned] (OCM-72) Remove dependency to cglib

2023-05-05 Thread Jira 


 [ 
https://issues.apache.org/jira/browse/OCM-72?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Michael Dürig reassigned OCM-72:


Assignee: Michael Dürig

> Remove dependency to cglib
> --
>
> Key: OCM-72
> URL: https://issues.apache.org/jira/browse/OCM-72
> Project: Jackrabbit OCM
>  Issue Type: Improvement
>Affects Versions: 1.6.0
>Reporter: Michael Dürig
>Assignee: Michael Dürig
>Priority: Major
>
> The [cglib|https://github.com/cglib/cglib] library does not work with Java 17 
> and should be replaced with something else. If there is some interest from 
> the community I could send a patch for migrating to 
> [ByteBuddy|https://bytebuddy.net/#/] based on some code for creating dynamic 
> proxies with [ProxyBuddy|https://github.com/mduerig/ProxyBuddy].



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Created] (OCM-72) Remove dependency to cglib

2023-05-05 Thread Jira 
Michael Dürig created OCM-72:


 Summary: Remove dependency to cglib
 Key: OCM-72
 URL: https://issues.apache.org/jira/browse/OCM-72
 Project: Jackrabbit OCM
  Issue Type: Improvement
Affects Versions: 1.6.0
Reporter: Michael Dürig


The [cglib|https://github.com/cglib/cglib] library does not work with Java 17 
and should be replaced with something else. If there is some interest from the 
community I could send a patch for migrating to 
[ByteBuddy|https://bytebuddy.net/#/] based on some code for creating dynamic 
proxies with [ProxyBuddy|https://github.com/mduerig/ProxyBuddy].



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

Re: Guava Migration (yes, a must read)

2023-05-05 Thread Julian Reschke 

On 06.04.2023 13:56, Julian Reschke wrote:

Hi everybody,

as some have noticed, we have begun the migration away from Guava 15.0
to a *shaded* version of Guava (latest and greatest).

A new subproject has been added (oak-shaded-guava), which repackages
Guava 31 under the package name o.a.jackrabbit.guava. See
 for details and
discussion.

If your IDE acts weird (be it Eclipse or IntelliJ), it's because it does
not understand the shaded subproject. Sorry for that. While there's no
fix for the IDEs, the workaround is to disable oak-shaded-guava
(Intellij) or remove it (Eclipse) - after having built it locally with
Maven.

We may decide at a later point to work around this by moving this out of
the reactor pom.

What's next?

1) We have a few sub projects that currently expose Guava in APIs; these
APIs have been deprecated for long, and I'll remove those APIs *after*
the next release (1.52.0).

2) In the meantime, everybody can help by converting "their" sub
projects. (If you do so, please add a sub-task to
). This usually means
adding the dependency to the POM, and rewriting import statements. I
usually just run:

~~~
#!/bin/bash

find . -name "*.java" -exec sed
"s/^import\scom\.google\.common\.\(.*;\)/import
org.apache.jackrabbit.guava.common.\1/g" -i {} ";"
find . -name "*.java" -exec sed
"s/^import\sstatic\scom\.google\.common\.\(.*;\)/import static
org.apache.jackrabbit.guava.common.\1/g" -i {} ";"
~~~

WARNING: this is destructive. Only run it on a Git checkout with no
local changes, and do not do it inside your project root. You have been
warned.

After running this, you may find that a Guava 15 method actually has
been removed. Usually googling for the method signature will tell you
what happened.
...


In the meantime, I have converted a good number of modules.

Namely missing are components from blobs, lucene, and sement storage,
where I would *really* appreciate that developers closer to that code
than me would step in.

For next week I'm planning to cut Oak 1.52.0. The plan is to remove
public APIs using native Guava by 1.54.0, and completely switch to
shaded Guava for internal use by 1.56.0.

Best regards, Julian

[GitHub] [jackrabbit-oak] reschke merged pull request #927: OAK-10231: switch oak-exercise to shaded guava

2023-05-05 Thread via GitHub 


reschke merged PR #927:
URL: https://github.com/apache/jackrabbit-oak/pull/927


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [jackrabbit-oak] steffenvan commented on a diff in pull request #914: OAK-10214: Expose node counter value as a metric in Oak

2023-05-05 Thread via GitHub 


steffenvan commented on code in PR #914:
URL: https://github.com/apache/jackrabbit-oak/pull/914#discussion_r1185814049


##
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/counter/NodeCounterEditor.java:
##
@@ -41,15 +44,20 @@
 public class NodeCounterEditor implements Editor {
 
 public static final String DATA_NODE_NAME = ":index";
-
+

Review Comment:
   Noted for the future! 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[jira] [Commented] (JCRVLT-683) Import of Authorizable node with acHandling=IGNORE should preserve existing rep:principalPolicy child node

2023-05-05 Thread Thomas Mueller (Jira) 


[ 
https://issues.apache.org/jira/browse/JCRVLT-683?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17719725#comment-17719725
 ] 

Thomas Mueller commented on JCRVLT-683:
---

> Well in this case I don't think it is necessary but if you want to add a 
> toggle please enrich the PR.

Well, that's not how reviews work... You don't ask the reviewer to add 
something that the author should do.

> Import of Authorizable node with acHandling=IGNORE should preserve existing 
> rep:principalPolicy child node
> --
>
> Key: JCRVLT-683
> URL: https://issues.apache.org/jira/browse/JCRVLT-683
> Project: Jackrabbit FileVault
>  Issue Type: Bug
>  Components: Packaging
>Affects Versions: 3.6.6
>Reporter: Mark Adamcin
>Assignee: Konrad Windszus
>Priority: Major
> Fix For: 3.6.10
>
>
> For situations where an authorizable node may be distributed from another 
> environment where a different rep:principalPolicy for the user is defined 
> than exists for that user in the target environment, it is important that the 
> existing rep:principalPolicy be preserved when acHandling is unset, 
> acHandling=IGNORE, or acHandling=MERGE_PRESERVE.
> Currently, the effective behavior of such a package install, as [it appears 
> to be implemented in 
> DocViewImporter|https://github.com/apache/jackrabbit-filevault/blob/5f9657374bd6c2d3dd1f6e9e2be0b9f5b25ddc26/vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java#L782-L787],
>  results in the following:
>  * If the package specifies acHandling=IGNORE, the existing 
> rep:principalPolicy is deleted without replacement, regardless of whether the 
> package contains its own rep:principalPolicy, which is equivalent to 
> *acHandling=CLEAR*
>  * If the package specifies acHandling=MERGE_PRESERVE or MERGE, the existing 
> rep:principalPolicy is replaced with whatever rep:principalPolicy is 
> contained in the package, or deletes the policy if a replacement is not 
> present, which is equivalent to *acHandling=OVERWRITE*
> Unexpectedly, the least destructive (and most default) acHandling mode 
> (IGNORE) turns out to be as destructive to packaged system user permissions 
> as choosing any other mode. 



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Comment Edited] (JCRVLT-683) Import of Authorizable node with acHandling=IGNORE should preserve existing rep:principalPolicy child node

2023-05-05 Thread Konrad Windszus (Jira) 


[ 
https://issues.apache.org/jira/browse/JCRVLT-683?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17719721#comment-17719721
 ] 

Konrad Windszus edited comment on JCRVLT-683 at 5/5/23 7:59 AM:


bq. https://github.com/apache/jackrabbit-filevault/pull/272 doesn't have 
reviewers. 

This is superseded by https://github.com/apache/jackrabbit-filevault/pull/294
In general I always add reviewer but they rarely react, therefore after some 
time I go ahead and merge. But everyone is involved to review, that includes 
you of course as well [~thomasm].

bq. Is it true that 
https://github.com/apache/jackrabbit-filevault/pull/294/files doesn't have any 
test case?

No, lots of ITs have been added 
(https://github.com/apache/jackrabbit-filevault/pull/294/files#diff-038077103e482e4a69670d008bbd1c150aab6edb9c6cb2d8e531940b094170e9)

bq. In general, I think that most bugfixes should come with "circuit breakers"

Well in this case I don't think it is necessary but if you want to add a toggle 
please enrich the PR.



was (Author: kwin):
bq. https://github.com/apache/jackrabbit-filevault/pull/272 doesn't have 
reviewers. In general I always add reviewer but they rarely react, therefore 
after some time I go ahead and merge. But everyone is involved to review, that 
includes you of course as well [~thomasm].

This is superseded by https://github.com/apache/jackrabbit-filevault/pull/294

bq. Is it true that 
https://github.com/apache/jackrabbit-filevault/pull/294/files doesn't have any 
test case?

No, lots of ITs have been added 
(https://github.com/apache/jackrabbit-filevault/pull/294/files#diff-038077103e482e4a69670d008bbd1c150aab6edb9c6cb2d8e531940b094170e9)

bq. In general, I think that most bugfixes should come with "circuit breakers"

Well in this case I don't think it is necessary but if you want to add a toggle 
please enrich the PR.


> Import of Authorizable node with acHandling=IGNORE should preserve existing 
> rep:principalPolicy child node
> --
>
> Key: JCRVLT-683
> URL: https://issues.apache.org/jira/browse/JCRVLT-683
> Project: Jackrabbit FileVault
>  Issue Type: Bug
>  Components: Packaging
>Affects Versions: 3.6.6
>Reporter: Mark Adamcin
>Assignee: Konrad Windszus
>Priority: Major
> Fix For: 3.6.10
>
>
> For situations where an authorizable node may be distributed from another 
> environment where a different rep:principalPolicy for the user is defined 
> than exists for that user in the target environment, it is important that the 
> existing rep:principalPolicy be preserved when acHandling is unset, 
> acHandling=IGNORE, or acHandling=MERGE_PRESERVE.
> Currently, the effective behavior of such a package install, as [it appears 
> to be implemented in 
> DocViewImporter|https://github.com/apache/jackrabbit-filevault/blob/5f9657374bd6c2d3dd1f6e9e2be0b9f5b25ddc26/vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java#L782-L787],
>  results in the following:
>  * If the package specifies acHandling=IGNORE, the existing 
> rep:principalPolicy is deleted without replacement, regardless of whether the 
> package contains its own rep:principalPolicy, which is equivalent to 
> *acHandling=CLEAR*
>  * If the package specifies acHandling=MERGE_PRESERVE or MERGE, the existing 
> rep:principalPolicy is replaced with whatever rep:principalPolicy is 
> contained in the package, or deletes the policy if a replacement is not 
> present, which is equivalent to *acHandling=OVERWRITE*
> Unexpectedly, the least destructive (and most default) acHandling mode 
> (IGNORE) turns out to be as destructive to packaged system user permissions 
> as choosing any other mode. 



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Comment Edited] (JCRVLT-683) Import of Authorizable node with acHandling=IGNORE should preserve existing rep:principalPolicy child node

2023-05-05 Thread Konrad Windszus (Jira) 


[ 
https://issues.apache.org/jira/browse/JCRVLT-683?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17719721#comment-17719721
 ] 

Konrad Windszus edited comment on JCRVLT-683 at 5/5/23 7:59 AM:


bq. https://github.com/apache/jackrabbit-filevault/pull/272 doesn't have 
reviewers. In general I always add reviewer but they rarely react, therefore 
after some time I go ahead and merge. But everyone is involved to review, that 
includes you of course as well [~thomasm].

This is superseded by https://github.com/apache/jackrabbit-filevault/pull/294

bq. Is it true that 
https://github.com/apache/jackrabbit-filevault/pull/294/files doesn't have any 
test case?

No, lots of ITs have been added 
(https://github.com/apache/jackrabbit-filevault/pull/294/files#diff-038077103e482e4a69670d008bbd1c150aab6edb9c6cb2d8e531940b094170e9)

bq. In general, I think that most bugfixes should come with "circuit breakers"

Well in this case I don't think it is necessary but if you want to add a toggle 
please enrich the PR.



was (Author: kwin):
bq. https://github.com/apache/jackrabbit-filevault/pull/272 doesn't have 
reviewers

This is superseded by https://github.com/apache/jackrabbit-filevault/pull/294

bq. Is it true that 
https://github.com/apache/jackrabbit-filevault/pull/294/files doesn't have any 
test case?

No, lots of ITs have been added 
(https://github.com/apache/jackrabbit-filevault/pull/294/files#diff-038077103e482e4a69670d008bbd1c150aab6edb9c6cb2d8e531940b094170e9)

bq. In general, I think that most bugfixes should come with "circuit breakers"

Well in this case I don't think it is necessary but if you want to add a toggle 
please enrich the PR.


> Import of Authorizable node with acHandling=IGNORE should preserve existing 
> rep:principalPolicy child node
> --
>
> Key: JCRVLT-683
> URL: https://issues.apache.org/jira/browse/JCRVLT-683
> Project: Jackrabbit FileVault
>  Issue Type: Bug
>  Components: Packaging
>Affects Versions: 3.6.6
>Reporter: Mark Adamcin
>Assignee: Konrad Windszus
>Priority: Major
> Fix For: 3.6.10
>
>
> For situations where an authorizable node may be distributed from another 
> environment where a different rep:principalPolicy for the user is defined 
> than exists for that user in the target environment, it is important that the 
> existing rep:principalPolicy be preserved when acHandling is unset, 
> acHandling=IGNORE, or acHandling=MERGE_PRESERVE.
> Currently, the effective behavior of such a package install, as [it appears 
> to be implemented in 
> DocViewImporter|https://github.com/apache/jackrabbit-filevault/blob/5f9657374bd6c2d3dd1f6e9e2be0b9f5b25ddc26/vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java#L782-L787],
>  results in the following:
>  * If the package specifies acHandling=IGNORE, the existing 
> rep:principalPolicy is deleted without replacement, regardless of whether the 
> package contains its own rep:principalPolicy, which is equivalent to 
> *acHandling=CLEAR*
>  * If the package specifies acHandling=MERGE_PRESERVE or MERGE, the existing 
> rep:principalPolicy is replaced with whatever rep:principalPolicy is 
> contained in the package, or deletes the policy if a replacement is not 
> present, which is equivalent to *acHandling=OVERWRITE*
> Unexpectedly, the least destructive (and most default) acHandling mode 
> (IGNORE) turns out to be as destructive to packaged system user permissions 
> as choosing any other mode. 



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Commented] (JCRVLT-683) Import of Authorizable node with acHandling=IGNORE should preserve existing rep:principalPolicy child node

2023-05-05 Thread Konrad Windszus (Jira) 


[ 
https://issues.apache.org/jira/browse/JCRVLT-683?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17719721#comment-17719721
 ] 

Konrad Windszus commented on JCRVLT-683:


bq. https://github.com/apache/jackrabbit-filevault/pull/272 doesn't have 
reviewers

This is superseded by https://github.com/apache/jackrabbit-filevault/pull/294

bq. Is it true that 
https://github.com/apache/jackrabbit-filevault/pull/294/files doesn't have any 
test case?

No, lots of ITs have been added 
(https://github.com/apache/jackrabbit-filevault/pull/294/files#diff-038077103e482e4a69670d008bbd1c150aab6edb9c6cb2d8e531940b094170e9)

bq. In general, I think that most bugfixes should come with "circuit breakers"

Well in this case I don't think it is necessary but if you want to add a toggle 
please enrich the PR.


> Import of Authorizable node with acHandling=IGNORE should preserve existing 
> rep:principalPolicy child node
> --
>
> Key: JCRVLT-683
> URL: https://issues.apache.org/jira/browse/JCRVLT-683
> Project: Jackrabbit FileVault
>  Issue Type: Bug
>  Components: Packaging
>Affects Versions: 3.6.6
>Reporter: Mark Adamcin
>Assignee: Konrad Windszus
>Priority: Major
> Fix For: 3.6.10
>
>
> For situations where an authorizable node may be distributed from another 
> environment where a different rep:principalPolicy for the user is defined 
> than exists for that user in the target environment, it is important that the 
> existing rep:principalPolicy be preserved when acHandling is unset, 
> acHandling=IGNORE, or acHandling=MERGE_PRESERVE.
> Currently, the effective behavior of such a package install, as [it appears 
> to be implemented in 
> DocViewImporter|https://github.com/apache/jackrabbit-filevault/blob/5f9657374bd6c2d3dd1f6e9e2be0b9f5b25ddc26/vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java#L782-L787],
>  results in the following:
>  * If the package specifies acHandling=IGNORE, the existing 
> rep:principalPolicy is deleted without replacement, regardless of whether the 
> package contains its own rep:principalPolicy, which is equivalent to 
> *acHandling=CLEAR*
>  * If the package specifies acHandling=MERGE_PRESERVE or MERGE, the existing 
> rep:principalPolicy is replaced with whatever rep:principalPolicy is 
> contained in the package, or deletes the policy if a replacement is not 
> present, which is equivalent to *acHandling=OVERWRITE*
> Unexpectedly, the least destructive (and most default) acHandling mode 
> (IGNORE) turns out to be as destructive to packaged system user permissions 
> as choosing any other mode. 



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Comment Edited] (JCRVLT-683) Import of Authorizable node with acHandling=IGNORE should preserve existing rep:principalPolicy child node

2023-05-05 Thread Thomas Mueller (Jira) 


[ 
https://issues.apache.org/jira/browse/JCRVLT-683?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17719719#comment-17719719
 ] 

Thomas Mueller edited comment on JCRVLT-683 at 5/5/23 7:49 AM:
---

> Every code relying on that bug can be easily fixed by adjusting the 
> aclHandling.

In general, I think that most bugfixes should come with "circuit breakers" 
(feature flags) that allow people to disable the "bugfix", specially if the 
bugfix changes behavior. In the past, we have used system properties to allow 
switching back to the old behavior. 

(There are bugfixes that _don't_ change the behavior, for example if they only 
improve performance.)

I also think that most changes should be reviewed. It seems the linked PRs 
don't currently have reviewers; is think the review process is done in some 
other way? Update: sorry, 
https://github.com/apache/jackrabbit-filevault/pull/272 doesn't have reviewers; 
https://github.com/apache/jackrabbit-filevault/pull/294 has reviewers.

PRs should also come with test cases. Is it true that 
https://github.com/apache/jackrabbit-filevault/pull/294/files doesn't have any 
test case?  There are a lot of code changes as far as I see.


was (Author: tmueller):
> Every code relying on that bug can be easily fixed by adjusting the 
> aclHandling.

In general, I think that most bugfixes should come with "circuit breakers" 
(feature flags) that allow people to disable the "bugfix", specially if the 
bugfix changes behavior. In the past, we have used system properties to allow 
switching back to the old behavior. 

(There are bugfixes that _don't_ change the behavior, for example if they only 
improve performance.)

I also think that most changes should be reviewed. It seems the linked PRs 
don't currently have reviewers; is think the review process is done in some 
other way?

PRs should also come with test cases. Is it true that 
https://github.com/apache/jackrabbit-filevault/pull/294/files doesn't have any 
test case?  There are a lot of code changes as far as I see.

> Import of Authorizable node with acHandling=IGNORE should preserve existing 
> rep:principalPolicy child node
> --
>
> Key: JCRVLT-683
> URL: https://issues.apache.org/jira/browse/JCRVLT-683
> Project: Jackrabbit FileVault
>  Issue Type: Bug
>  Components: Packaging
>Affects Versions: 3.6.6
>Reporter: Mark Adamcin
>Assignee: Konrad Windszus
>Priority: Major
> Fix For: 3.6.10
>
>
> For situations where an authorizable node may be distributed from another 
> environment where a different rep:principalPolicy for the user is defined 
> than exists for that user in the target environment, it is important that the 
> existing rep:principalPolicy be preserved when acHandling is unset, 
> acHandling=IGNORE, or acHandling=MERGE_PRESERVE.
> Currently, the effective behavior of such a package install, as [it appears 
> to be implemented in 
> DocViewImporter|https://github.com/apache/jackrabbit-filevault/blob/5f9657374bd6c2d3dd1f6e9e2be0b9f5b25ddc26/vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java#L782-L787],
>  results in the following:
>  * If the package specifies acHandling=IGNORE, the existing 
> rep:principalPolicy is deleted without replacement, regardless of whether the 
> package contains its own rep:principalPolicy, which is equivalent to 
> *acHandling=CLEAR*
>  * If the package specifies acHandling=MERGE_PRESERVE or MERGE, the existing 
> rep:principalPolicy is replaced with whatever rep:principalPolicy is 
> contained in the package, or deletes the policy if a replacement is not 
> present, which is equivalent to *acHandling=OVERWRITE*
> Unexpectedly, the least destructive (and most default) acHandling mode 
> (IGNORE) turns out to be as destructive to packaged system user permissions 
> as choosing any other mode. 



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Commented] (JCRVLT-683) Import of Authorizable node with acHandling=IGNORE should preserve existing rep:principalPolicy child node

2023-05-05 Thread Thomas Mueller (Jira) 


[ 
https://issues.apache.org/jira/browse/JCRVLT-683?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17719719#comment-17719719
 ] 

Thomas Mueller commented on JCRVLT-683:
---

> Every code relying on that bug can be easily fixed by adjusting the 
> aclHandling.

In general, I think that most bugfixes should come with "circuit breakers" 
(feature flags) that allow people to disable the "bugfix", specially if the 
bugfix changes behavior. In the past, we have used system properties to allow 
switching back to the old behavior. 

(There are bugfixes that _don't_ change the behavior, for example if they only 
improve performance.)

I also think that most changes should be reviewed. It seems the linked PRs 
don't currently have reviewers; is think the review process is done in some 
other way?

PRs should also come with test cases. Is it true that 
https://github.com/apache/jackrabbit-filevault/pull/294/files doesn't have any 
test case?  There are a lot of code changes as far as I see.

> Import of Authorizable node with acHandling=IGNORE should preserve existing 
> rep:principalPolicy child node
> --
>
> Key: JCRVLT-683
> URL: https://issues.apache.org/jira/browse/JCRVLT-683
> Project: Jackrabbit FileVault
>  Issue Type: Bug
>  Components: Packaging
>Affects Versions: 3.6.6
>Reporter: Mark Adamcin
>Assignee: Konrad Windszus
>Priority: Major
> Fix For: 3.6.10
>
>
> For situations where an authorizable node may be distributed from another 
> environment where a different rep:principalPolicy for the user is defined 
> than exists for that user in the target environment, it is important that the 
> existing rep:principalPolicy be preserved when acHandling is unset, 
> acHandling=IGNORE, or acHandling=MERGE_PRESERVE.
> Currently, the effective behavior of such a package install, as [it appears 
> to be implemented in 
> DocViewImporter|https://github.com/apache/jackrabbit-filevault/blob/5f9657374bd6c2d3dd1f6e9e2be0b9f5b25ddc26/vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java#L782-L787],
>  results in the following:
>  * If the package specifies acHandling=IGNORE, the existing 
> rep:principalPolicy is deleted without replacement, regardless of whether the 
> package contains its own rep:principalPolicy, which is equivalent to 
> *acHandling=CLEAR*
>  * If the package specifies acHandling=MERGE_PRESERVE or MERGE, the existing 
> rep:principalPolicy is replaced with whatever rep:principalPolicy is 
> contained in the package, or deletes the policy if a replacement is not 
> present, which is equivalent to *acHandling=OVERWRITE*
> Unexpectedly, the least destructive (and most default) acHandling mode 
> (IGNORE) turns out to be as destructive to packaged system user permissions 
> as choosing any other mode. 



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[GitHub] [jackrabbit-oak] reschke merged pull request #926: OAK-10230: switch oak-authorization-principalbased to shaded guava

2023-05-05 Thread via GitHub 


reschke merged PR #926:
URL: https://github.com/apache/jackrabbit-oak/pull/926


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [jackrabbit-oak] reschke commented on a diff in pull request #926: OAK-10230: switch oak-authorization-principalbased to shaded guava

2023-05-05 Thread via GitHub 


reschke commented on code in PR #926:
URL: https://github.com/apache/jackrabbit-oak/pull/926#discussion_r1185746647


##
oak-authorization-principalbased/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/AbstractEntry.java:
##
@@ -16,7 +16,7 @@
  */
 package 
org.apache.jackrabbit.oak.spi.security.authorization.principalbased.impl;
 
-import com.google.common.base.Objects;
+import org.apache.jackrabbit.guava.common.base.Objects;

Review Comment:
   It's a specific method in "Objects" that moved to "MoreObjects". Others 
stayed where they were.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [jackrabbit-oak] anchela commented on a diff in pull request #926: OAK-10230: switch oak-authorization-principalbased to shaded guava

2023-05-05 Thread via GitHub 


anchela commented on code in PR #926:
URL: https://github.com/apache/jackrabbit-oak/pull/926#discussion_r1185745561


##
oak-authorization-principalbased/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/ImmutablePrincipalPolicy.java:
##
@@ -16,7 +16,7 @@
  */
 package 
org.apache.jackrabbit.oak.spi.security.authorization.principalbased.impl;
 
-import com.google.common.base.Objects;
+import org.apache.jackrabbit.guava.common.base.Objects;

Review Comment:
   same here



##
oak-authorization-principalbased/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/AbstractEntry.java:
##
@@ -16,7 +16,7 @@
  */
 package 
org.apache.jackrabbit.oak.spi.security.authorization.principalbased.impl;
 
-import com.google.common.base.Objects;
+import org.apache.jackrabbit.guava.common.base.Objects;

Review Comment:
   in other PR this was replaced by MoreObjects, no?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[jira] [Comment Edited] (JCRVLT-700) package creation fails for node names that are valid in Oak, but invalid in Jackrabbit

2023-05-05 Thread Julian Reschke (Jira) 


[ 
https://issues.apache.org/jira/browse/JCRVLT-700?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17706381#comment-17706381
 ] 

Julian Reschke edited comment on JCRVLT-700 at 5/5/23 6:26 AM:
---

Added test coverage in subtask in JCRVLT-701.

Confirmed this can be solved by tuning jcr-spi-commons, for which I'll add a 
separate ticket.


was (Author: reschke):
Added test coverage in subtask in JCRVLT-701.

Confirmed this can be solved by tunning jcr-spi-commons, for which I'll add a 
separate ticket.

> package creation fails for node names that are valid in Oak, but invalid in 
> Jackrabbit
> --
>
> Key: JCRVLT-700
> URL: https://issues.apache.org/jira/browse/JCRVLT-700
> Project: Jackrabbit FileVault
>  Issue Type: Bug
>  Components: vlt
>Reporter: Julian Reschke
>Assignee: Julian Reschke
>Priority: Major
> Attachments: JCRVLT-700-test.diff
>
>
> FV uses jackrabbit-spi-commons to parse node names (for instance, to get the 
> namespace name and the local name). However, Jackrabbit is more strict with 
> respect to non-ASCII whitespace characters than Oak.
> This leads to a situation where nodes using these characters can not be 
> packaged.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)