You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by GitBox <gi...@apache.org> on 2022/02/08 14:17:54 UTC
[GitHub] [jackrabbit-oak] joerghoh opened a new pull request #485: SLING-9612 Convert ReadOnlyBuilder's exceptions into RepositoryExcept…
joerghoh opened a new pull request #485:
URL: https://github.com/apache/jackrabbit-oak/pull/485
…ions
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] joerghoh commented on a change in pull request #485: OAK-9612 Convert ReadOnlyBuilder's exceptions into RepositoryExcept…
Posted by GitBox <gi...@apache.org>.
joerghoh commented on a change in pull request #485:
URL: https://github.com/apache/jackrabbit-oak/pull/485#discussion_r804543811
##########
File path: oak-lucene/src/test/java/org/apache/jackrabbit/oak/composite/blueGreen/IndexUtils.java
##########
@@ -142,7 +142,7 @@ public static void checkLibsIsReadOnly(Persistence p) throws RepositoryException
try {
libsNode.addNode("illegal");
Assert.fail();
- } catch (UnsupportedOperationException e) {
+ } catch (RepositoryException e) {
Review comment:
That is an implementation detail of oak-jcr which I don't want to leak into into oak-lucene.
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] mreutegg commented on a change in pull request #485: OAK-9612 Convert ReadOnlyBuilder's exceptions into RepositoryExcept…
Posted by GitBox <gi...@apache.org>.
mreutegg commented on a change in pull request #485:
URL: https://github.com/apache/jackrabbit-oak/pull/485#discussion_r804778450
##########
File path: oak-lucene/src/test/java/org/apache/jackrabbit/oak/composite/blueGreen/IndexUtils.java
##########
@@ -142,7 +142,7 @@ public static void checkLibsIsReadOnly(Persistence p) throws RepositoryException
try {
libsNode.addNode("illegal");
Assert.fail();
- } catch (UnsupportedOperationException e) {
+ } catch (RepositoryException e) {
Review comment:
Fair enough. Though, the test did specifically check for just this implementation detail that an UnsupportedOperationException is thrown when an attempt is made to add a node on a read-only mount.
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] anchela commented on a change in pull request #485: OAK-9612 Convert ReadOnlyBuilder's exceptions into RepositoryExcept…
Posted by GitBox <gi...@apache.org>.
anchela commented on a change in pull request #485:
URL: https://github.com/apache/jackrabbit-oak/pull/485#discussion_r801684504
##########
File path: oak-store-spi/src/main/java/org/apache/jackrabbit/oak/spi/state/ReadyOnlyBuilderUnsupportedOperationException.java
##########
@@ -0,0 +1,12 @@
+package org.apache.jackrabbit.oak.spi.state;
+
+/**
+ * Indicates that a modification operation was tried to execute on a read-only builder.
+ * It should be used instead of throwing plain UnsupportedOperationExceptions in that situation.
+ */
+public class ReadyOnlyBuilderUnsupportedOperationException extends UnsupportedOperationException {
Review comment:
Maybe `ReadyOnlyBuilderException` would be sufficiently clear and a bit shorter than `ReadyOnlyBuilderUnsupportedOperationException`
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] anchela merged pull request #485: OAK-9612 Convert ReadOnlyBuilder's exceptions into RepositoryExcept…
Posted by GitBox <gi...@apache.org>.
anchela merged pull request #485:
URL: https://github.com/apache/jackrabbit-oak/pull/485
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] joerghoh commented on a change in pull request #485: OAK-9612 Convert ReadOnlyBuilder's exceptions into RepositoryExcept…
Posted by GitBox <gi...@apache.org>.
joerghoh commented on a change in pull request #485:
URL: https://github.com/apache/jackrabbit-oak/pull/485#discussion_r804460757
##########
File path: oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/delegate/SessionDelegate.java
##########
@@ -212,6 +213,8 @@ public void refreshAtNextAccess() {
} finally {
postPerform(sessionOperation, t0);
}
+ } catch (ReadyOnlyBuilderException e) {
+ throw new RepositoryException(e);
Review comment:
We actually discussed it in the OAK-9612 to throw a ```ConstraintViolationException```. Will change it.
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] joerghoh commented on a change in pull request #485: OAK-9612 Convert ReadOnlyBuilder's exceptions into RepositoryExcept…
Posted by GitBox <gi...@apache.org>.
joerghoh commented on a change in pull request #485:
URL: https://github.com/apache/jackrabbit-oak/pull/485#discussion_r801877522
##########
File path: oak-store-spi/src/main/java/org/apache/jackrabbit/oak/spi/state/ReadyOnlyBuilderUnsupportedOperationException.java
##########
@@ -0,0 +1,12 @@
+package org.apache.jackrabbit.oak.spi.state;
+
+/**
+ * Indicates that a modification operation was tried to execute on a read-only builder.
+ * It should be used instead of throwing plain UnsupportedOperationExceptions in that situation.
+ */
+public class ReadyOnlyBuilderUnsupportedOperationException extends UnsupportedOperationException {
Review comment:
done
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] mreutegg commented on a change in pull request #485: OAK-9612 Convert ReadOnlyBuilder's exceptions into RepositoryExcept…
Posted by GitBox <gi...@apache.org>.
mreutegg commented on a change in pull request #485:
URL: https://github.com/apache/jackrabbit-oak/pull/485#discussion_r803817011
##########
File path: oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/delegate/SessionDelegate.java
##########
@@ -212,6 +213,8 @@ public void refreshAtNextAccess() {
} finally {
postPerform(sessionOperation, t0);
}
+ } catch (ReadyOnlyBuilderException e) {
+ throw new RepositoryException(e);
Review comment:
What is the rationale behind throwing a RepositoryException vs ConstraintViolationException?
I'm not saying it's wrong. Just want to understand the reason why one is used instead of the other.
##########
File path: oak-lucene/src/test/java/org/apache/jackrabbit/oak/composite/blueGreen/IndexUtils.java
##########
@@ -142,7 +142,7 @@ public static void checkLibsIsReadOnly(Persistence p) throws RepositoryException
try {
libsNode.addNode("illegal");
Assert.fail();
- } catch (UnsupportedOperationException e) {
+ } catch (RepositoryException e) {
Review comment:
I think it would be better if the test also check if the cause of the RepositoryException is a ReadOnlyBuilderException.
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] anchela merged pull request #485: OAK-9612 Convert ReadOnlyBuilder's exceptions into RepositoryExcept…
Posted by GitBox <gi...@apache.org>.
anchela merged pull request #485:
URL: https://github.com/apache/jackrabbit-oak/pull/485
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] joerghoh commented on pull request #485: OAK-9612 Convert ReadOnlyBuilder's exceptions into RepositoryExcept…
Posted by GitBox <gi...@apache.org>.
joerghoh commented on pull request #485:
URL: https://github.com/apache/jackrabbit-oak/pull/485#issuecomment-1032660886
(when merging please adjust the jira ID in the commit message ... of course it's OAK-9612!)
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] mreutegg commented on a change in pull request #485: OAK-9612 Convert ReadOnlyBuilder's exceptions into RepositoryExcept…
Posted by GitBox <gi...@apache.org>.
mreutegg commented on a change in pull request #485:
URL: https://github.com/apache/jackrabbit-oak/pull/485#discussion_r804802274
##########
File path: oak-lucene/src/test/java/org/apache/jackrabbit/oak/composite/blueGreen/IndexUtils.java
##########
@@ -142,7 +142,7 @@ public static void checkLibsIsReadOnly(Persistence p) throws RepositoryException
try {
libsNode.addNode("illegal");
Assert.fail();
- } catch (UnsupportedOperationException e) {
+ } catch (RepositoryException e) {
Review comment:
> This test is not responsible to test the implementation of the JCR, so it should just rely on the API contract.
Well, currently the test does check an implementation detail. It catches a UnsupportedOperationException. But you are right the method does not `UnsupportedOperationException` nor `ConstraintViolationException`.
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] joerghoh commented on pull request #485: OAK-9612 Convert ReadOnlyBuilder's exceptions into RepositoryExcept…
Posted by GitBox <gi...@apache.org>.
joerghoh commented on pull request #485:
URL: https://github.com/apache/jackrabbit-oak/pull/485#issuecomment-1036082822
@mreutegg I added additional test coverage and changed the exception as indicated.
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] mreutegg commented on a change in pull request #485: OAK-9612 Convert ReadOnlyBuilder's exceptions into RepositoryExcept…
Posted by GitBox <gi...@apache.org>.
mreutegg commented on a change in pull request #485:
URL: https://github.com/apache/jackrabbit-oak/pull/485#discussion_r804780607
##########
File path: oak-lucene/src/test/java/org/apache/jackrabbit/oak/composite/blueGreen/IndexUtils.java
##########
@@ -142,7 +142,7 @@ public static void checkLibsIsReadOnly(Persistence p) throws RepositoryException
try {
libsNode.addNode("illegal");
Assert.fail();
- } catch (UnsupportedOperationException e) {
+ } catch (RepositoryException e) {
Review comment:
Catch `ConstraintViolationException` instead?
##########
File path: oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/delegate/SessionDelegateTest.java
##########
@@ -68,4 +74,38 @@ public void testRefreshUnaware() {
delegate.refresh(false);
verify(pp, times(6)).refresh();
}
+
+ @Test(expected = RepositoryException.class)
Review comment:
Shouldn't these tests now expect a `ConstraintViolationException`?
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] joerghoh commented on a change in pull request #485: OAK-9612 Convert ReadOnlyBuilder's exceptions into RepositoryExcept…
Posted by GitBox <gi...@apache.org>.
joerghoh commented on a change in pull request #485:
URL: https://github.com/apache/jackrabbit-oak/pull/485#discussion_r804790962
##########
File path: oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/delegate/SessionDelegateTest.java
##########
@@ -68,4 +74,38 @@ public void testRefreshUnaware() {
delegate.refresh(false);
verify(pp, times(6)).refresh();
}
+
+ @Test(expected = RepositoryException.class)
Review comment:
The contract states just a ```RepositoryException```, so it makes sense just to test for that.
Although one could argue, that a change in that code, which also throws a ```RepositoryException``` (or an exception inheriting from it) would be caught as well.
As this is the test for this implementation,I will change it to the ```ConstraintViolationException```.
##########
File path: oak-lucene/src/test/java/org/apache/jackrabbit/oak/composite/blueGreen/IndexUtils.java
##########
@@ -142,7 +142,7 @@ public static void checkLibsIsReadOnly(Persistence p) throws RepositoryException
try {
libsNode.addNode("illegal");
Assert.fail();
- } catch (UnsupportedOperationException e) {
+ } catch (RepositoryException e) {
Review comment:
Won't change it here. This test is not responsible to test the implementation of the JCR, so it should just rely on the API contract.
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] joerghoh commented on pull request #485: OAK-9612 Convert ReadOnlyBuilder's exceptions into RepositoryExcept…
Posted by GitBox <gi...@apache.org>.
joerghoh commented on pull request #485:
URL: https://github.com/apache/jackrabbit-oak/pull/485#issuecomment-1036447040
@mreutegg / @anchela: I don't have enough karma/permissions to merge this PR, can you do it for me?
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org