[
https://issues.apache.org/jira/browse/OAK-7570?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16539320#comment-16539320
]
Alexander Klimetschek edited comment on OAK-7570 at 7/10/18 10:51 PM:
----------------------------------------------------------------------
[~mduerig]
{quote}Adding a pair of such methods per implementation does certainly pollute
these APIs and create undesirable strong coupling.
{quote}
It's in the generic parts of Oak that only have one implementation (oak-jcr
SessionImpl, oak-api MutableRoot), and then in the NodeStores, BlobStores and
DataStores that choose to provide this implementation.
To me (with hindsight of course :) this is a feature that was "missed" by JCR,
and really should be part of JCR (if there would be a 2.x version) . Hence it
finds its way into different places of Oak...
Having said that, if my
[questions|https://github.com/apache/jackrabbit-oak/pull/94#pullrequestreview-136033951]
on the whiteboard approach can get a "is ok" answer, I don't want to object to
this approach if you think it's better for maintaining Oak.
{quote}This is not what the JavaDoc of HttpBinaryProvider#initiateHttpUpload()
implies though.
{quote}
What do you think is the [current
javadoc|https://github.com/mattvryan/jackrabbit-oak/blob/b2afdc74a0e3f2fbc0c6debb1f3bf9205a9e4d6a/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/api/binary/HttpBinaryProvider.java#L101-L106]
implying that is a problem? The javadoc is geared towards the client. It
should not fully define the implementation approach, if there can be variations
in the future (say a new repository wide "upload binary" privilege for
example). Happy to improve the docs.
{quote}Oak's access control model is powerful enough to address this by adding
proper permission checks
{quote}
This is a proper permission check using Oak's existing access control
mechanisms... checking if the user can set the binary property. We are _not_
inventing anything new here.
We simply replicate something that was previously a single request (upload
binary) and single JCR session operation (set binary property + save), into two
separate requests (initiate + complete). Where the initiate step must fail
already if the permission is not right.
If we deem to change the implementation in the future to make the check based
on a new repository wide privilege for example, or some other approach, it's
not a problem if the client passes the path argument here. It could just be
ignored. However, if we turn it around and start with no path argument and no
check, but later want to introduce it, we would have to change the API and
force all clients to change.
The path to the property, the session and the intention to write a binary is
the most specific information for this operation AFAICS, so I don't see any
other information that might be needed for a different permission check
approach in the future.
was (Author: alexander.klimetschek):
[~mduerig]
{quote}Adding a pair of such methods per implementation does certainly pollute
these APIs and create undesirable strong coupling.
{quote}
It's in the generic parts of Oak that only have one implementation (oak-jcr
SessionImpl, oak-api MutableRoot), and then in the NodeStores, BlobStores and
DataStores that choose to provide this implementation.
To me (with hindsight of course :) this is a feature that was "missed" by JCR,
and really should be part of JCR (if there would be a 2.x version) . Hence it
finds its way into different places of Oak...
Having said that, if my
[questions|https://github.com/apache/jackrabbit-oak/pull/94#pullrequestreview-136033951]
on the whiteboard approach can get a "is ok" answer, I don't want to object to
this approach if you think it's better for maintaining Oak.
{quote}This is not what the JavaDoc of HttpBinaryProvider#initiateHttpUpload()
implies though.
{quote}
What do you think is the [current
javadoc|https://github.com/mattvryan/jackrabbit-oak/blob/b2afdc74a0e3f2fbc0c6debb1f3bf9205a9e4d6a/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/api/binary/HttpBinaryProvider.java#L101-L106]
implying that is a problem? The javadoc is geared towards the client. It
should not fully define the implementation approach, if there can be variations
in the future (say a new repository wide "upload binary" privilege for
example). Happy to improve the docs.
{quote}Oak's access control model is powerful enough to address this by adding
proper permission checks
{quote}
This is a proper permission check using Oak's existing access control
mechanisms... checking if the user can set the binary property. We are _not_
inventing anything new here.
We simply replicate something that was previously a single request (upload
binary) and single JCR session operation (set binary property + save), into two
separate requests (initiate + complete). Where the initiate step must fail
already if the permission is not right.
If we deem to change the implementation in the future to make the check based
on a new repository wide privilege for example, or some other approach, it's
not a problem if the client passes the path argument here. It could just be
ignored. However, if we turn it around and start with no path argument and no
check, but later want to introduce it, we would have to change the API and
force all clients to change.
> [DirectBinaryAccess][DISCUSS] How to access HttpBlobProvider from oak-jcr
> -------------------------------------------------------------------------
>
> Key: OAK-7570
> URL: https://issues.apache.org/jira/browse/OAK-7570
> Project: Jackrabbit Oak
> Issue Type: Technical task
> Components: jcr
> Reporter: Matt Ryan
> Assignee: Matt Ryan
> Priority: Major
>
> Open discussion related to OAK-7569:
> The [original pull request|https://github.com/apache/jackrabbit-oak/pull/88]
> proposes changes to oak-api, oak-segment-tar, oak-store-document, oak-core,
> and oak-jcr as well as oak-blob-plugins, oak-blob-cloud, and oak-blob-azure.
> Would it be possible / better to keep the changes local to the oak-blob-*
> bundles and avoid making changes throughout the stack?
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
