[jira] [Commented] (SLING-12279) Move Sling Model cache holder for Resource and ResourceResolver adaptables into the resource resolver property map
[
https://issues.apache.org/jira/browse/SLING-12279?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17846767#comment-17846767
]
Paul Bjorkstrand commented on SLING-12279:
--
[~pakira], you are correct that this does not fix that particular problem. The
problem that this attempts to address, is when the cache fills with
[circularly-referenced cached
models|https://sling.apache.org/documentation/bundles/models.html#a-note-about-cache-true-and-using-the-self-injector]
(most commonly when model marked with `cache=true`, and keeps a field with
`@Self`), this change should allow the GC to collect the cache when the
Resource Resolver or Request are closed/destroyed. That will eliminate a large
"sawtooth" GC graph.
Without this change, models that, directly or indirectly, reference their
adaptable will not be collected until there is memory pressure, and the GC
starts collecting SoftReference. I do believe with my proposed change, swapping
out the cache impl (currently just a `Map,
SoftReference>>` will be easier. A follow-on change could add in the
LRU portion of the cache impl in one place, rather than in the several it would
need today..
> Move Sling Model cache holder for Resource and ResourceResolver adaptables
> into the resource resolver property map
> --
>
> Key: SLING-12279
> URL: https://issues.apache.org/jira/browse/SLING-12279
> Project: Sling
> Issue Type: Improvement
> Components: Sling Models
>Affects Versions: Models Implementation 1.6.4
>Reporter: Paul Bjorkstrand
>Priority: Major
> Attachments: cache-size.log, cachesize.log
>
>
> There have been several instances of issues with model caching over the
> years, the most recent being SLING-12259 and [this
> thread|https://www.mail-archive.com/dev@sling.apache.org/msg131926.html] from
> Jörg Hoh. These recent issues have been around cached items sticking
> around for too long. In that thread, it was discussed using
> {{[ResourceResolver#getPropertyMap()|https://github.com/apache/sling-org-apache-sling-api/blob/master/src/main/java/org/apache/sling/api/resource/ResourceResolver.java#L888]}}
> whenever possible. This binds the cache to the lifecycle of the
> ResourceResolver, avoiding the global cache altogether.
> After looking at the [current
> implementation|https://github.com/apache/sling-org-apache-sling-models-impl/blob/master/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java#L341],
> there is already a divergence that binds the cache for request-based models
> to the request, by putting the cache in the request attribute.
> This proposes to do a similar change for Resource/Resolver based models, and
> use the resolver's property map to bind the cache for those adaptables to the
> lifecycle of the ResourceResolver. Resources are already (largely) bound to
> the lifecycle of the ResourceResolver that supplied them, and binding the
> models that come from these Resources seems to be a reasonable approach.
> This will greatly reduce the lifetime of model objects, reducing the
> likelihood of the JVM's GC being put under pressure in cases when cached
> models [reference the original adaptable using
> {{@Self}}|https://sling.apache.org/documentation/bundles/models.html#a-note-about-cache-true-and-using-the-self-injector].
> As a bonus, if the cache object implements {{Closeable}}, the Sling
> Implementation will call {{close()}} on the cache when the resolver is
> closed,, giving us further control over the lifetime of objects in the cache.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-12279) Move Sling Model cache holder for Resource and ResourceResolver adaptables into the resource resolver property map
[
https://issues.apache.org/jira/browse/SLING-12279?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17841341#comment-17841341
]
Paul Bjorkstrand commented on SLING-12279:
--
[~joerghoh], can you take a look at this and see if it solves your problem? I
believe it will, but I don't have a way to reliably test. I could try to
contrive some kind of unit test that does some verification, though it may
prove difficult to make. Since the GC is largely non-deterministic (from an
outsider's perspective), getting this into a field test would likely be better.
> Move Sling Model cache holder for Resource and ResourceResolver adaptables
> into the resource resolver property map
> --
>
> Key: SLING-12279
> URL: https://issues.apache.org/jira/browse/SLING-12279
> Project: Sling
> Issue Type: Improvement
> Components: Sling Models
>Affects Versions: Models Implementation 1.6.4
>Reporter: Paul Bjorkstrand
>Priority: Major
>
> There have been several instances of issues with model caching over the
> years, the most recent being SLING-12259 and [this
> thread|https://www.mail-archive.com/dev@sling.apache.org/msg131926.html] from
> Jörg Hoh. These recent issues have been around cached items sticking
> around for too long. In that thread, it was discussed using
> {{[ResourceResolver#getPropertyMap()|https://github.com/apache/sling-org-apache-sling-api/blob/master/src/main/java/org/apache/sling/api/resource/ResourceResolver.java#L888]}}
> whenever possible. This binds the cache to the lifecycle of the
> ResourceResolver, avoiding the global cache altogether.
> After looking at the [current
> implementation|https://github.com/apache/sling-org-apache-sling-models-impl/blob/master/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java#L341],
> there is already a divergence that binds the cache for request-based models
> to the request, by putting the cache in the request attribute.
> This proposes to do a similar change for Resource/Resolver based models, and
> use the resolver's property map to bind the cache for those adaptables to the
> lifecycle of the ResourceResolver. Resources are already (largely) bound to
> the lifecycle of the ResourceResolver that supplied them, and binding the
> models that come from these Resources seems to be a reasonable approach.
> This will greatly reduce the lifetime of model objects, reducing the
> likelihood of the JVM's GC being put under pressure in cases when cached
> models [reference the original adaptable using
> {{@Self}}|https://sling.apache.org/documentation/bundles/models.html#a-note-about-cache-true-and-using-the-self-injector].
> As a bonus, if the cache object implements {{Closeable}}, the Sling
> Implementation will call {{close()}} on the cache when the resolver is
> closed,, giving us further control over the lifetime of objects in the cache.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-12300) Provide a way to retrieve a JCR backed resource by its node identifier
[
https://issues.apache.org/jira/browse/SLING-12300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17840127#comment-17840127
]
Paul Bjorkstrand commented on SLING-12300:
--
That's a good analysis, [~radu], thanks for sharing. I understand the Resource
API challenges. It makes sense to keep this as a JCR concept. I feel we can
drop that idea from contention given its significant constraints.
Regarding the other two, maybe there is a way to be able to have the best of
both worlds, by making the new {{/jcr:id/}} path available, but behind a
configuration flag ({{false}} by default). I think that would allow you to keep
the simpler implementation, but also addresses any security/exposure related
issues, since it would be an opt-in feature.
Thoughts?
> Provide a way to retrieve a JCR backed resource by its node identifier
> --
>
> Key: SLING-12300
> URL: https://issues.apache.org/jira/browse/SLING-12300
> Project: Sling
> Issue Type: New Feature
> Components: JCR
>Reporter: Radu Cotescu
>Assignee: Radu Cotescu
>Priority: Major
> Fix For: JCR Resource 3.3.0
>
>
> Since all {{javax.jcr.Nodes}} have an identifier [0], a useful feature would
> be {{Resource}} retrieval by node id, which could be its {{jcr:uuid}}
> property for referenceable nodes or the path. In systems that would like to
> use UUID addressing, this would reduce the need for executing JCR queries for
> resource retrieval and would avoid double-reads via the JCR and then Sling
> API to obtain the resource.
> In order to provide a unified behaviour, paths starting with the {{/jcr:id/}}
> prefix should use the resource retrieval by node identifier.
> [0] -
> https://javadoc.io/static/javax.jcr/jcr/2.0/javax/jcr/Node.html#getIdentifier()
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-12300) Provide a way to retrieve a JCR backed resource by its node identifier
[
https://issues.apache.org/jira/browse/SLING-12300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17839419#comment-17839419
]
Paul Bjorkstrand commented on SLING-12300:
--
I apologize if I am perceived as trivializing; I am trying to understand your
concern, but I am not able to find any reason (other than security/privacy) why
having the UUIDs addressable is a problem. I do want to understand what the
concern about having some kind of predictability is, though.
Based on part of your previous comments, I would like to address some of the
concerns strictly related to predictability.
bq. Basically, I'm not putting a server on the public internet with predictable
addresses.
I don't think that predictability is necessarily a problem in itself. In many
situations (especially for things based on Sling), predictability is not a
detriment but a feature. I know that there are Sling implementations that are
not AEM, but using AEM as an example, you already have a large swath of
partially predictable addresses. Public data primarily lives under
{{/content}}. (Mostly private) user data lives under {{/home}}, (almost
entirely private) application data/code lives under {{/apps}}. These root
segments are entirely predictable.
If you will oblige, I would like to run through an example comparing UUIDs vs.
paths in terms of predictability, starting with some assumptions:
# The "root path segments" in a given implementation are entirely predictable.
# Paths consist of characters found in the following regex: {{[A-Za-z0-9-/]}}.
This gives you 64 characters to choose from (more or fewer characters could be
used, and these seem common enough in URLs).
#* I chose this character set because it made the math easier, since 64 is a
power of 2.
#* More characters could be used, but it doesn't really change the math much.
# Paths of nodes are entirely random, using the character set above.
# Every node in a given Sling instance has {{mix:referenceable}} applied and is
provisioned a UUID.
# Paths could be any random combination of the characters from the regex above.
In theory, a path could be of unlimited length, while UUIDs are finite, that is
true. Using the assumptions above, for a path to be less predictable than a
UUID, it would need to be at least than 21 characters long _beyond any
predictable root(s)_. I won't go into the math too deeply, but the point where
64^x^ crosses 2^122^ is approximately 20.41. Anything 20 characters or less is
more prone to brute force attacks than a UUID!
_Note, even if you use all 8 bits of ASCII, you get 256^x^, which makes the
length needed 16 characters (~15.25).
In practice, paths are almost never random strings of characters. They have
meaning in their names, often semantic and syntactic restrictions (depending on
the creator's language). System rules can also reduce what paths are allowed
(e.g., {{//}} is not legal because of the sequential slashes). Additionally,
paths are usually relatively short. My experience tells me that paths are
rarely more than 50 characters long in the public part of the application.
Lastly, not every node in a given instance is going to have
{{mix:referenceable}}, and thus not every node has a UUID that could be
brute-forced.
If your concern is not security or privacy related can help me understand what
it is [~enorman]?
> Provide a way to retrieve a JCR backed resource by its node identifier
> --
>
> Key: SLING-12300
> URL: https://issues.apache.org/jira/browse/SLING-12300
> Project: Sling
> Issue Type: New Feature
> Components: JCR
>Reporter: Radu Cotescu
>Assignee: Radu Cotescu
>Priority: Major
> Fix For: JCR Resource 3.3.0
>
>
> Since all {{javax.jcr.Nodes}} have an identifier [0], a useful feature would
> be {{Resource}} retrieval by node id, which could be its {{jcr:uuid}}
> property for referenceable nodes or the path. In systems that would like to
> use UUID addressing, this would reduce the need for executing JCR queries for
> resource retrieval and would avoid double-reads via the JCR and then Sling
> API to obtain the resource.
> In order to provide a unified behaviour, paths starting with the {{/jcr:id/}}
> prefix should use the resource retrieval by node identifier.
> [0] -
> https://javadoc.io/static/javax.jcr/jcr/2.0/javax/jcr/Node.html#getIdentifier()
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-12300) Provide a way to retrieve a JCR backed resource by its node identifier
[
https://issues.apache.org/jira/browse/SLING-12300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17839397#comment-17839397
]
Paul Bjorkstrand commented on SLING-12300:
--
To clarify my position:
I am not against having a mechanism to access them via identifier. My argument
is mostly around the 'magic" path of {{/jcr:id/*}}. To me, I feel using a new
method (as mentioned above) to perform this function. If you are creating an
application that works outside the typical Sling hierarchical retrieve, it
should probably be a new primitive in Sling, and not comingled with the
existing {{getResource()}}.
An argument for this functionality might be: Sling already have vanity paths
that muddy this concept, so what is the concern for another?
About the security argument, I think it is fair to say that we all know that
"security by obscurity" is no security at all. Just because there is some means
to access nodes by some ID that has a smaller universe of unique identifiers
compared to the path does not change any of the security
characteristics...unless you are using obscurity as your security mechanism.
As Radu stated, the security checks performed are still the same ones that
would be performed on the target node/resource, so there is no real security
weakening here.
While I still think that adding the magic path is still a possible source of
confusion, I am not so against it after some deeper thinking on it. I still
believe that a new method would be better, but it seems reasonable, especially
when compared side-by-side with vanities.
Would creating a new resource provider actually duplicate a lot? Or would you
be able to build this as an additional provider in the same bundle as the JCR
Resource Provider, and use the same functionality directly?
> Provide a way to retrieve a JCR backed resource by its node identifier
> --
>
> Key: SLING-12300
> URL: https://issues.apache.org/jira/browse/SLING-12300
> Project: Sling
> Issue Type: New Feature
> Components: JCR
>Reporter: Radu Cotescu
>Assignee: Radu Cotescu
>Priority: Major
> Fix For: JCR Resource 3.3.0
>
>
> Since all {{javax.jcr.Nodes}} have an identifier [0], a useful feature would
> be {{Resource}} retrieval by node id, which could be its {{jcr:uuid}}
> property for referenceable nodes or the path. In systems that would like to
> use UUID addressing, this would reduce the need for executing JCR queries for
> resource retrieval and would avoid double-reads via the JCR and then Sling
> API to obtain the resource.
> In order to provide a unified behaviour, paths starting with the {{/jcr:id/}}
> prefix should use the resource retrieval by node identifier.
> [0] -
> https://javadoc.io/static/javax.jcr/jcr/2.0/javax/jcr/Node.html#getIdentifier()
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-12300) Provide a way to retrieve a JCR backed resource by its node identifier
[
https://issues.apache.org/jira/browse/SLING-12300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17839116#comment-17839116
]
Paul Bjorkstrand commented on SLING-12300:
--
I'm leaning towards [~joerghoh]'s opinion as well. To me, this seems better
served by a new method like {{ResourceResolver#getResoureById(String jcrId);}}
Having this functionality in {{getResource(..)}} method seems to be a possible
source for confusion.
If I were to add in something that provides resources under the path
{{/jcr:id/*}}, it almost seems like it should be from a provider that services
that path. That may also bring into it more problems, because I don't know if
it is semantically correct for a resource provider under a given path to
provide resources for another path.
Is there a reason why this can't be a new method?
> Provide a way to retrieve a JCR backed resource by its node identifier
> --
>
> Key: SLING-12300
> URL: https://issues.apache.org/jira/browse/SLING-12300
> Project: Sling
> Issue Type: New Feature
> Components: JCR
>Reporter: Radu Cotescu
>Assignee: Radu Cotescu
>Priority: Major
> Fix For: JCR Resource 3.3.0
>
>
> Since all {{javax.jcr.Nodes}} have an identifier [0], a useful feature would
> be {{Resource}} retrieval by node id, which could be its {{jcr:uuid}}
> property for referenceable nodes or the path. In systems that would like to
> use UUID addressing, this would reduce the need for executing JCR queries for
> resource retrieval and would avoid double-reads via the JCR and then Sling
> API to obtain the resource.
> In order to provide a unified behaviour, paths starting with the {{/jcr:id/}}
> prefix should use the resource retrieval by node identifier.
> [0] -
> https://javadoc.io/static/javax.jcr/jcr/2.0/javax/jcr/Node.html#getIdentifier()
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Created] (SLING-12279) Move Sling Model cache holder for Resource and ResourceResolver adaptables into the resource resolver property map
Paul Bjorkstrand created SLING-12279:
Summary: Move Sling Model cache holder for Resource and
ResourceResolver adaptables into the resource resolver property map
Key: SLING-12279
URL: https://issues.apache.org/jira/browse/SLING-12279
Project: Sling
Issue Type: Improvement
Components: Sling Models
Affects Versions: Models Implementation 1.6.4
Reporter: Paul Bjorkstrand
There have been several instances of issues with model caching over the years,
the most recent being SLING-12259 and [this
thread|https://www.mail-archive.com/dev@sling.apache.org/msg131926.html] from
Jörg Hoh. These recent issues have been around cached items sticking
around for too long. In that thread, it was discussed using
{{[ResourceResolver#getPropertyMap()|https://github.com/apache/sling-org-apache-sling-api/blob/master/src/main/java/org/apache/sling/api/resource/ResourceResolver.java#L888]}}
whenever possible. This binds the cache to the lifecycle of the
ResourceResolver, avoiding the global cache altogether.
After looking at the [current
implementation|https://github.com/apache/sling-org-apache-sling-models-impl/blob/master/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java#L341],
there is already a divergence that binds the cache for request-based models to
the request, by putting the cache in the request attribute.
This proposes to do a similar change for Resource/Resolver based models, and
use the resolver's property map to bind the cache for those adaptables to the
lifecycle of the ResourceResolver. Resources are already (largely) bound to the
lifecycle of the ResourceResolver that supplied them, and binding the models
that come from these Resources seems to be a reasonable approach.
This will greatly reduce the lifetime of model objects, reducing the likelihood
of the JVM's GC being put under pressure in cases when cached models [reference
the original adaptable using
{{@Self}}|https://sling.apache.org/documentation/bundles/models.html#a-note-about-cache-true-and-using-the-self-injector].
As a bonus, if the cache object implements {{Closeable}}, the Sling
Implementation will call {{close()}} on the cache when the resolver is closed,,
giving us further control over the lifetime of objects in the cache.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-8069) Sling Models: Enable constructor injection to use non-public constructors
[
https://issues.apache.org/jira/browse/SLING-8069?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17459254#comment-17459254
]
Paul Bjorkstrand commented on SLING-8069:
-
Absolutely, [~kwin]. PR coming shortly for sling-site.
> Sling Models: Enable constructor injection to use non-public constructors
> -
>
> Key: SLING-8069
> URL: https://issues.apache.org/jira/browse/SLING-8069
> Project: Sling
> Issue Type: Improvement
> Components: Sling Models
>Affects Versions: Models Impl 1.4.10
>Reporter: Paul Bjorkstrand
>Assignee: Konrad Windszus
>Priority: Minor
> Fix For: Models Implementation 1.5.0
>
> Time Spent: 2h
> Remaining Estimate: 0h
>
> In Sling Models, you cannot use a non-public constructor to inject. Looking
> through the code, there doesn't seem to be any clear reason for this
> restriction. In my opinion, constructor injection should allow any
> constructor visibility.
> Here are some discussion points:
> * Private fields are allowed for use, so disallowing private constructors
> seems unnecessary.
> * Private constructors may be bad practice (difficult to test), but Sling
> should not be telling users how to write their Java code. This is especially
> true for models, since it should work with POJOs, as stated in the
> documentation. It would be trivial to add checks to just allow default,
> protected, or public, but I feel that logic is unnecessary.
> * Non-public methods could also be allowed, but that can be a separate
> ticket.
> ** A prerequisite of this would be to allow setter injection on models in
> the first place. Again, not the subject of this ticket.
> * Threading concerns are minimal, but there could be possible deadlocks, as
> with any multi-threaded application that uses locks. In general, I think
> locking similar to how it is done in {{InjectableField}} would be sufficient.
> The risk of deadlock would be similar to the risk of the locking in
> {{injectableField.set(Object, Result)}}.
>
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
[jira] [Commented] (SLING-10969) Remove synchronized & rest of accessible flag during injection
[
https://issues.apache.org/jira/browse/SLING-10969?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17454090#comment-17454090
]
Paul Bjorkstrand commented on SLING-10969:
--
[~sseifert], [~rombert], [~kwin]
PR: https://github.com/apache/sling-org-apache-sling-models-impl/pull/29
> Remove synchronized & rest of accessible flag during injection
> --
>
> Key: SLING-10969
> URL: https://issues.apache.org/jira/browse/SLING-10969
> Project: Sling
> Issue Type: Improvement
> Components: Sling Models
>Affects Versions: Models Implementation 1.4.16
>Reporter: Paul Bjorkstrand
>Priority: Major
>
> To improve multithreaded performance of Sling Models, the {{synchronized}}
> blocks, along with the "reset" of the {{setAccessible}} using its original
> value should be removed.
> Context:
> The synchronized blocks were added to resolve a race condition [1]. After
> looking into another large project that uses reflection to access members of
> classes (Felix [2]), nowhere in that project is something similar being done.
> Every reflective access is just doing {{setAccessible(true)}}.
> Results from a JMH test allude to a significant performance improvement
> during multithreaded threaded access by removing the synchronized (JMH
> results [3]).
> [1]: SLING-6584
> [2]: https://github.com/apache/felix-dev/search?q=setAccessible
> [3]: https://gist.github.com/paul-bjorkstrand/f3bb154665e7d2168b4656eb7b794496
> [4]: https://www.mail-archive.com/dev@sling.apache.org/msg113123.html
> [5]: https://github.com/apache/sling-org-apache-sling-models-impl/pull/11
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
[jira] [Created] (SLING-10969) Remove synchronized & rest of accessible flag during injection
Paul Bjorkstrand created SLING-10969:
Summary: Remove synchronized & rest of accessible flag during
injection
Key: SLING-10969
URL: https://issues.apache.org/jira/browse/SLING-10969
Project: Sling
Issue Type: Improvement
Components: Sling Models
Affects Versions: Models Implementation 1.4.16
Reporter: Paul Bjorkstrand
To improve multithreaded performance of Sling Models, the {{synchronized}}
blocks, along with the "reset" of the {{setAccessible}} using its original
value should be removed.
Context:
The synchronized blocks were added to resolve a race condition [1]. After
looking into another large project that uses reflection to access members of
classes (Felix [2]), nowhere in that project is something similar being done.
Every reflective access is just doing {{setAccessible(true)}}.
Results from a JMH test allude to a significant performance improvement during
multithreaded threaded access by removing the synchronized (JMH results [3]).
[1]: SLING-6584
[2]: https://github.com/apache/felix-dev/search?q=setAccessible
[3]: https://gist.github.com/paul-bjorkstrand/f3bb154665e7d2168b4656eb7b794496
[4]: https://www.mail-archive.com/dev@sling.apache.org/msg113123.html
[5]: https://github.com/apache/sling-org-apache-sling-models-impl/pull/11
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
[jira] [Commented] (SLING-10947) Sling Models Unit Tests do not compile with Java 11
[ https://issues.apache.org/jira/browse/SLING-10947?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17451277#comment-17451277 ] Paul Bjorkstrand commented on SLING-10947: -- I just noticed, this ticket is somewhat a duplicate of SLING-10037., maybe we can close this one in favor of that one? > Sling Models Unit Tests do not compile with Java 11 > --- > > Key: SLING-10947 > URL: https://issues.apache.org/jira/browse/SLING-10947 > Project: Sling > Issue Type: Bug > Components: Sling Models >Affects Versions: Models Implementation 1.4.16 >Reporter: Stefan Seifert >Priority: Minor > Fix For: Models Implementation 1.5.0 > > > due to a reference to sun.misc in > https://github.com/apache/sling-org-apache-sling-models-impl/blob/a016b05184a17f3760f8594139fabc573d5bfdae/src/test/java/org/apache/sling/models/impl/AdapterFactoryTest.java#L313-L335 -- This message was sent by Atlassian Jira (v8.20.1#820001)
[jira] [Commented] (SLING-10947) Sling Models Unit Tests do not compile with Java 11
[ https://issues.apache.org/jira/browse/SLING-10947?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17451275#comment-17451275 ] Paul Bjorkstrand commented on SLING-10947: -- I had an inspiration, so this evening became this morning :) https://github.com/apache/sling-org-apache-sling-models-impl/pull/25 It looks like there is a sonar issue (https://ci-builds.apache.org/blue/organizations/jenkins/Sling%2Fmodules%2Fsling-org-apache-sling-models-impl/detail/PR-25/1/pipeline/33/), that seems to be unrelated to the changes. Can you take a look [~sseifert] (or if you know who can, bring 'em in to the conversation)? > Sling Models Unit Tests do not compile with Java 11 > --- > > Key: SLING-10947 > URL: https://issues.apache.org/jira/browse/SLING-10947 > Project: Sling > Issue Type: Bug > Components: Sling Models >Affects Versions: Models Implementation 1.4.16 >Reporter: Stefan Seifert >Priority: Minor > Fix For: Models Implementation 1.5.0 > > > due to a reference to sun.misc in > https://github.com/apache/sling-org-apache-sling-models-impl/blob/a016b05184a17f3760f8594139fabc573d5bfdae/src/test/java/org/apache/sling/models/impl/AdapterFactoryTest.java#L313-L335 -- This message was sent by Atlassian Jira (v8.20.1#820001)
[jira] [Commented] (SLING-10947) Sling Models Unit Tests do not compile with Java 11
[
https://issues.apache.org/jira/browse/SLING-10947?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17451242#comment-17451242
]
Paul Bjorkstrand commented on SLING-10947:
--
Having been the one to write that test, I think we can refactor it to avoid the
need to use {{Unsafe}}. Lemme mull on it for the day and I'll see if I can get
an MR up this evening.
> Sling Models Unit Tests do not compile with Java 11
> ---
>
> Key: SLING-10947
> URL: https://issues.apache.org/jira/browse/SLING-10947
> Project: Sling
> Issue Type: Bug
> Components: Sling Models
>Affects Versions: Models Implementation 1.4.16
>Reporter: Stefan Seifert
>Priority: Minor
> Fix For: Models Implementation 1.5.0
>
>
> due to a reference to sun.misc in
> https://github.com/apache/sling-org-apache-sling-models-impl/blob/a016b05184a17f3760f8594139fabc573d5bfdae/src/test/java/org/apache/sling/models/impl/AdapterFactoryTest.java#L313-L335
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
[jira] [Commented] (SLING-10946) slingsettings deprecated what should i now use?
[ https://issues.apache.org/jira/browse/SLING-10946?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17450780#comment-17450780 ] Paul Bjorkstrand commented on SLING-10946: -- [~paul@gmail.com], the [SlingSettingsService|https://github.com/apache/sling-org-apache-sling-settings/blob/master/src/main/java/org/apache/sling/settings/SlingSettingsService.java] is not deprecated as far as I am aware. I imagine you are talking about [AEMaaCS' version of that API|https://javadoc.io/doc/com.adobe.aem/aem-sdk-api/latest/org/apache/sling/settings/SlingSettingsService.html] being deprecated. That is not part of Apache Sling, though. Regardless, this is not the proper venue for the question. If it is an AEM specific question, I recommend Adobe's Experience League community. Also, you can join us on the [mailing lists|https://sling.apache.org/project-information.html#mailing-lists] for Apache Sling related questions. Cheers! > slingsettings deprecated what should i now use? > --- > > Key: SLING-10946 > URL: https://issues.apache.org/jira/browse/SLING-10946 > Project: Sling > Issue Type: Bug > Components: Sling Models >Reporter: Paul W >Priority: Minor > > not necessarily a bug, but now that slingsettings is deprecated, we are not > able to get the runmode in the sling model, what is the alternative to this? -- This message was sent by Atlassian Jira (v8.20.1#820001)
[jira] [Commented] (SLING-10150) Sling Resource Merger completely hides parent when whitelisting in combination with asterisk is used for sling:hideChildren
[
https://issues.apache.org/jira/browse/SLING-10150?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17287319#comment-17287319
]
Paul Bjorkstrand commented on SLING-10150:
--
Just a question for [~Henry Kuijpers], have you tried changing the order of the
hide-children values? If the {{!desired-tab}} is evaluated first then the {{*}}
is next, maybe it is an ordering problem (or maybe I am completely missing part
of the problem).
> Sling Resource Merger completely hides parent when whitelisting in
> combination with asterisk is used for sling:hideChildren
> ---
>
> Key: SLING-10150
> URL: https://issues.apache.org/jira/browse/SLING-10150
> Project: Sling
> Issue Type: Bug
>Affects Versions: Resource Merger 1.3.10
>Reporter: Henry Kuijpers
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Provided a failing test case here:
> https://github.com/apache/sling-org-apache-sling-resourcemerger/pull/3/files
> TODO: Fix
> When trying to configure a whitelisting approach to inheriting nodes from a
> parent (i.e. through resource super type, or through overlaying), the
> following way:
> /apps/base/test/tabs/undesired-tab
> /apps/base/test/tabs/desired-tab
> /apps/base/test/tabs/desired-tab/items/field@title="title"
> &
> /apps/overlay/test/tabs@sling:hideChildren="[!desired-tab,*]"
> /apps/overlay/test/tabs/desired-tab/items/field@description="test"
> One would expect that requesting the children of /merged/test/tabs would
> yield the "desired-tab" only, i.e. "undesired-tab" (and other nodes not
> whitelisted) being hidden. This is working as expected.
> One would also expect the "desired-tab" to have the properties of the
> base-structure as well as the properties of the overlay-structure. This is
> also working as expected.
> One would expect that the underlying nodes of "desired-tab" from the base
> would remain intact and would be merged with the underlying nodes of
> "desired-tab" in the overlay. So, while listing the items of desired-tab, one
> would expect:
> MergedResource containing properties [title=test, description=test]
> consisting of original resources
> [/apps/base/test/tabs/desired-tab/items/field,
> /apps/overlay/test/tabs/desired-tab/items/field]
> However, instead, the following is returned:
> MergedResource containing properties [description=test] consisting of
> original resources [/apps/overlay/test/tabs/desired-tab/items/field]
> So, the original "base" resource is not considered anymore!
> I believe the issue is in MergingResourceProvider.ParentHidingHandler, in the
> constructor, actually. At some point, it decides that the parent resource
> (which indeed has the sling:hideChildren property defined) defines an exclude
> entry which is "*" and adds that entry to the list.
> Then, when the class starts checking the parent of the parent (marked with
> "// also check on the parent's parent whether that was hiding the parent") -
> There it will find that asterisk exclude that was defined on the parent, not
> taking into account that it was preceded by a whitelisting "!desired-tab" -
> Removing the parent of the parent's children entirely.
> I believe this should be changed into a more robust way of handling this
> use-case. Probably the asterisk exclude can be global(?), even though it
> should still be desired that any child of the parent still is able to remove
> that exclude. But whenever those excludes are considered, also the includes
> that were preceding it should be considered to figure out if it's a real
> include in the case of that specific path.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-10113) data-sly-resource throwing RecursionTooDeepException
[
https://issues.apache.org/jira/browse/SLING-10113?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17276645#comment-17276645
]
Paul Bjorkstrand commented on SLING-10113:
--
Does {{targetPagePath}} start with a slash ({{/}})? Per the code [1], the path
is appended to the current resource, if it does not
[1]:
https://github.com/apache/sling-org-apache-sling-scripting-sightly/blob/org.apache.sling.scripting.sightly-1.1.2-1.4.0/src/main/java/org/apache/sling/scripting/sightly/impl/engine/extension/ResourceRuntimeExtension.java#L212
> data-sly-resource throwing RecursionTooDeepException
>
>
> Key: SLING-10113
> URL: https://issues.apache.org/jira/browse/SLING-10113
> Project: Sling
> Issue Type: Bug
> Components: Scripting
>Reporter: Marvin Flock
>Priority: Major
> Attachments: image-2021-02-01-17-40-19-873.png
>
>
> AEM Version 6.5.7
> While using data-sly-resource with a path, I get a RecursionTooDeepException.
> See -SLING-7685- for Similarity
> I currently cant make hold of the behavior, using
> __
> will include the current page infinite times (at least it tries) instead of
> the target page
> while using something like this:
> __
> will render the curentPage with the RecursionTooDeepException.
> The HTL Engine I am using is of the following version:
> !image-2021-02-01-17-40-19-873.png!
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-10011) Use javax.jcr.Item.getParent() when resolving parent JCR node in JcrResourceProvider#getParent
[
https://issues.apache.org/jira/browse/SLING-10011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17253778#comment-17253778
]
Paul Bjorkstrand commented on SLING-10011:
--
I think the point that [~cziegeler] and [~jsedding] are making is that the
session _should_ have enough information to optimize the call
{code:java}
((JackrabbitSession) session).getItemOrNull(path);{code}
which is made in [JcrResourceitemFactory, line 187 (at time of
writing)|https://github.com/apache/sling-org-apache-sling-jcr-resource/blob/master/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrItemResourceFactory.java#L187].
If an improvement is needed, it should be made in Oak for two reasons:
# Sling should not need to use a specific call to get performance from the JCR
(except in unusual edge cases).
# If an improvement is made in Oak, then all projects that use Oak benefit,
including Sling.
While that may increase the effort of this, it puts the effort in the correct
location: at the database.
Think of it in terms analogous to RDBMS: you would not expect to use one type
of SQL query if the database was split across many disks, and another if it
were on a single SSD. You would expect the database to have knowledge of its
logical and physical structure, so it can optimize its own operations. That is
the abstraction a database user would expect.
> Use javax.jcr.Item.getParent() when resolving parent JCR node in
> JcrResourceProvider#getParent
> --
>
> Key: SLING-10011
> URL: https://issues.apache.org/jira/browse/SLING-10011
> Project: Sling
> Issue Type: Improvement
> Components: JCR
>Affects Versions: JCR Resource 3.0.22
>Reporter: Miroslav Smiljanic
>Priority: Minor
> Time Spent: 20m
> Remaining Estimate: 0h
>
> Currently
> [JcrResourceProvider.getParent|https://github.com/apache/sling-org-apache-sling-jcr-resource/blob/org.apache.sling.jcr.resource-3.0.22/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProvider.java#L361]
> is using JcrItemResourceFactory.getItemOrNull(String path), which eventually
> is using JCR session to retrieve parent node using absolute path.
> I propose using javax.jcr.Item.getParent() instead.
> Reasoning wold be to utilise potential improvements in JCR implementation
> that would for a given node retrieve the whole subtree. That can be
> configured for example by using particular node type or node path.
> {noformat}
> root
> |
> a
>/ \
> b c
> {noformat}
> If node 'a' in picture above, is matching desired configuration, then code
> below would return the whole subtree.
> {code:java}
> Node a = jcrSession.getNode("a");
> {code}
> That further means retrieved subtree can be traversed in memory, without the
> need to communicate with the JCR repository storage.
> (!)That is particularly important when remote (cloud) storage is used for
> repository in JCR implementation, and tree traversal can be done without
> doing additional network roundtrips.
> {code:java}
> //JCR tree traversal happens in memory
> Node b = a.getNode("b");
> Node c = a.getNode("c");
> {code}
> Also going from child to parent, is resolved in memory as well (proposal
> relates to this fact)
> {code:java}
> //JCR tree traversal happens in memory
> assert b.getParent() == c.getParent();
> {code}
> Jackrabbit Oak, for document node store is supporting node bundling for
> configured node type
> [http://jackrabbit.apache.org/oak/docs/nodestore/document/node-bundling.html]
> Currently I am also doing some experiments to support node
> bundling/aggregation for arbitrary node store
> ([NodeDelegateFullyLoaded|https://github.com/smiroslav/jackrabbit-oak/blob/ppnextgen_newstore/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/delegate/NodeDelegateFullyLoaded.java],
>
> [FullyLoadedTree|https://github.com/smiroslav/jackrabbit-oak/blob/ppnextgen_newstore/oak-core/src/main/java/org/apache/jackrabbit/oak/core/FullyLoadedTree.java]).
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-9829) data-sly-element should correctly handle void elements
[
https://issues.apache.org/jira/browse/SLING-9829?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17219080#comment-17219080
]
Paul Bjorkstrand commented on SLING-9829:
-
Thanks for the explanation. Even if it were just the element name, that would
be helpful. I understand that it would be nontrivial, but it would probably be
useful to add in some kind of logging facility in the future. I added
SLING-9849 to request that feature.
Thanks again, [~radu].
> data-sly-element should correctly handle void elements
> --
>
> Key: SLING-9829
> URL: https://issues.apache.org/jira/browse/SLING-9829
> Project: Sling
> Issue Type: Bug
> Components: Scripting
>Affects Versions: Scripting Sightly Engine 1.0.0, Scripting HTL Compiler
> 1.0.0, Scripting HTL Compiler 1.1.0-1.4.0, Scripting HTL Compiler 1.2.0-1.4.0
>Reporter: Radu Cotescu
>Assignee: Radu Cotescu
>Priority: Major
> Fix For: Scripting HTL Compiler 1.2.12-1.4.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> The current implementation of {{data-sly-element}} doesn't correctly handle
> void elements which will replace the original element on which the block
> element was placed:
> When {{$\{item.element.name}}} evaluates to {{link}} or {{meta}}, the element
> will have a closing tag in the following example, although both {{link}} and
> {{meta}} are void elements:
> {code:html}
>
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Created] (SLING-9849) Add logging functionality to the compiler and runtime to support better error notification for the developer
Paul Bjorkstrand created SLING-9849: --- Summary: Add logging functionality to the compiler and runtime to support better error notification for the developer Key: SLING-9849 URL: https://issues.apache.org/jira/browse/SLING-9849 Project: Sling Issue Type: Wish Components: Scripting Reporter: Paul Bjorkstrand As a developer, it would be helpful to have more detailed logging in compiled HTL scripts to assist with troubleshooting issues in the HTL script logic. SLING-9829 exposed the fact that there is no runtime support for logging errors or warnings with any degree of specificity. To implement this would likely require (at minimum), changes to the HTL compiler and HTL runtime. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (SLING-9829) data-sly-element should correctly handle void elements
[
https://issues.apache.org/jira/browse/SLING-9829?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17214717#comment-17214717
]
Paul Bjorkstrand commented on SLING-9829:
-
Though it would be bead form, I am curious how you will handle when the element
name is for a void element, but the actual element has child nodes (text
content or other elements). It might be worthwhile to at least debug log when
that situation occurs, something along the lines of "The name in
data-sly-element is a void element, all children of this element
will be ignored/removed"
Thoughts?
> data-sly-element should correctly handle void elements
> --
>
> Key: SLING-9829
> URL: https://issues.apache.org/jira/browse/SLING-9829
> Project: Sling
> Issue Type: Bug
> Components: Scripting
>Affects Versions: Scripting Sightly Engine 1.0.0, Scripting HTL Compiler
> 1.0.0, Scripting HTL Compiler 1.1.0-1.4.0, Scripting HTL Compiler 1.2.0-1.4.0
>Reporter: Radu Cotescu
>Assignee: Radu Cotescu
>Priority: Major
> Fix For: Scripting HTL Compiler 1.2.12-1.4.0
>
>
> The current implementation of {{data-sly-element}} doesn't correctly handle
> void elements which will replace the original element on which the block
> element was placed:
> When {{$\{item.element.name}}} evaluates to {{link}} or {{meta}}, the element
> will have a closing tag in the following example, although both {{link}} and
> {{meta}} are void elements:
> {code:html}
>
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Comment Edited] (SLING-9829) data-sly-element should correctly handle void elements
[
https://issues.apache.org/jira/browse/SLING-9829?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17214717#comment-17214717
]
Paul Bjorkstrand edited comment on SLING-9829 at 10/15/20, 2:00 PM:
Though it would be bad form to add children to an eventual void element, I am
curious how you will handle when the element name is for a void element, but
the actual element has child nodes (text content or other elements). It might
be worthwhile to at least debug log when that situation occurs, something along
the lines of "The name in data-sly-element is a void element,
all children of this element will be ignored/removed"
Thoughts?
was (Author: paul.bjorkstrand):
Though it would be bead form, I am curious how you will handle when the element
name is for a void element, but the actual element has child nodes (text
content or other elements). It might be worthwhile to at least debug log when
that situation occurs, something along the lines of "The name in
data-sly-element is a void element, all children of this element
will be ignored/removed"
Thoughts?
> data-sly-element should correctly handle void elements
> --
>
> Key: SLING-9829
> URL: https://issues.apache.org/jira/browse/SLING-9829
> Project: Sling
> Issue Type: Bug
> Components: Scripting
>Affects Versions: Scripting Sightly Engine 1.0.0, Scripting HTL Compiler
> 1.0.0, Scripting HTL Compiler 1.1.0-1.4.0, Scripting HTL Compiler 1.2.0-1.4.0
>Reporter: Radu Cotescu
>Assignee: Radu Cotescu
>Priority: Major
> Fix For: Scripting HTL Compiler 1.2.12-1.4.0
>
>
> The current implementation of {{data-sly-element}} doesn't correctly handle
> void elements which will replace the original element on which the block
> element was placed:
> When {{$\{item.element.name}}} evaluates to {{link}} or {{meta}}, the element
> will have a closing tag in the following example, although both {{link}} and
> {{meta}} are void elements:
> {code:html}
>
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Created] (SLING-9715) Sling models JavaUseProvider does not properly handle the adaptable argument for Sling Model classes
Paul Bjorkstrand created SLING-9715:
---
Summary: Sling models JavaUseProvider does not properly handle the
adaptable argument for Sling Model classes
Key: SLING-9715
URL: https://issues.apache.org/jira/browse/SLING-9715
Project: Sling
Issue Type: Bug
Components: Sling Models
Affects Versions: Scripting HTL Engine 1.4.0-1.4.0
Reporter: Paul Bjorkstrand
When Sling Models instantiation was added in SLING-9320
([commit|https://github.com/apache/sling-org-apache-sling-scripting-sightly/commit/bcd46027e09182911c55bb244d42debb1bd367c7]),
it did not treat the {{adaptable}} argument the same for all types of
instantiations (Sling Models vs basic Sling Adaptable). This fixes that by
rearranging the instantiation attempt order, which puts the argument provided
adaptable through {{ModelFactory#createModel}} just like the request and
resource would be.
This is a subtle semantic change, and likely warrants a minor version update.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Comment Edited] (SLING-9314) NPE in ObjectModel.toBoolean(Object) when object.toString() returns null
[ https://issues.apache.org/jira/browse/SLING-9314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17185209#comment-17185209 ] Paul Bjorkstrand edited comment on SLING-9314 at 8/26/20, 2:56 PM: --- [~radu], I added a comment (rather lengthy, sorry) to the PR with an idea on how to both simplify the logic and to make it more readable. There is one non-backwards compatible change in the suggestion, though I feel it is worth eliminating that edge case. Let me know your thoughts on the impacts of that change. was (Author: paul.bjorkstrand): [~radu], I add a comment (rather lengthy, sorry) to the PR with an idea on how to both simplify the logic and to make it more readable. There is one non-backwards compatible change in the suggestion, though I feel it is worth eliminating that edge case. Let me know your thoughts on the impacts of that change. > NPE in ObjectModel.toBoolean(Object) when object.toString() returns null > > > Key: SLING-9314 > URL: https://issues.apache.org/jira/browse/SLING-9314 > Project: Sling > Issue Type: Bug > Components: Scripting >Affects Versions: Scripting HTL Runtime 1.0.0-1.4.0, Scripting HTL Runtime > 1.1.0-1.4.0, Scripting HTL Runtime 1.1.2-1.4.0 >Reporter: Paul Bjorkstrand >Assignee: Radu Cotescu >Priority: Major > Fix For: Scripting HTL Runtime 1.2.4-1.4.0 > > > Though it is bad practice, it is possible that an object can return null from > its toString() method. ObjectModel.toBoolean(Object) \[[Line > 161|https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/blob/master/src/main/java/org/apache/sling/scripting/sightly/render/ObjectModel.java#L161]\] > calls .trim() on a potentially null object. > This causes a difficult to troubleshoot, deeply nested, and cryptic exception > to be raised. If object.toString() returns null, then it should be treated > the same as nearly the rest of HTL, where null is considered "falsey". Doing > so will save hours of difficult troubleshooting. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (SLING-9314) NPE in ObjectModel.toBoolean(Object) when object.toString() returns null
[ https://issues.apache.org/jira/browse/SLING-9314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17185209#comment-17185209 ] Paul Bjorkstrand commented on SLING-9314: - [~radu], I add a comment (rather lengthy, sorry) to the PR with an idea on how to both simplify the logic and to make it more readable. There is one non-backwards compatible change in the suggestion, though I feel it is worth eliminating that edge case. Let me know your thoughts on the impacts of that change. > NPE in ObjectModel.toBoolean(Object) when object.toString() returns null > > > Key: SLING-9314 > URL: https://issues.apache.org/jira/browse/SLING-9314 > Project: Sling > Issue Type: Bug > Components: Scripting >Affects Versions: Scripting HTL Runtime 1.0.0-1.4.0, Scripting HTL Runtime > 1.1.0-1.4.0, Scripting HTL Runtime 1.1.2-1.4.0 >Reporter: Paul Bjorkstrand >Assignee: Radu Cotescu >Priority: Major > Fix For: Scripting HTL Runtime 1.2.4-1.4.0 > > > Though it is bad practice, it is possible that an object can return null from > its toString() method. ObjectModel.toBoolean(Object) \[[Line > 161|https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/blob/master/src/main/java/org/apache/sling/scripting/sightly/render/ObjectModel.java#L161]\] > calls .trim() on a potentially null object. > This causes a difficult to troubleshoot, deeply nested, and cryptic exception > to be raised. If object.toString() returns null, then it should be treated > the same as nearly the rest of HTL, where null is considered "falsey". Doing > so will save hours of difficult troubleshooting. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (SLING-9314) HTL null pointer in ObjectModel.toBoolean(Object) when object.toString() returns null
[
https://issues.apache.org/jira/browse/SLING-9314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17184103#comment-17184103
]
Paul Bjorkstrand commented on SLING-9314:
-
That works too, [~radu]. This was mainly a "quick get the details down" kind of
report, that I did not yet follow up on. The suggestion for falsey was based on
my specific situation. It might be worthwhile to add a warn/error log when
toString returns null.
As I stated in the first sentence, it is bad practice (and ostensibly breaks
the contract of {{#toString}}) but that does not stop bad code from sneaking
in. Thinking for both allowing further processing and helping with
troubleshooting something like this maybe (pseudocode):
{code}
s = o.toString();
if (s == null) {
log warn or error with as much detail as possible
s = EMPTY
}
s = s.trim()
... the rest of the logic ...
{code}
> HTL null pointer in ObjectModel.toBoolean(Object) when object.toString()
> returns null
> -
>
> Key: SLING-9314
> URL: https://issues.apache.org/jira/browse/SLING-9314
> Project: Sling
> Issue Type: Bug
> Components: Scripting
>Affects Versions: Scripting HTL Runtime 1.0.0-1.4.0, Scripting HTL Runtime
> 1.1.0-1.4.0, Scripting HTL Runtime 1.1.2-1.4.0
>Reporter: Paul Bjorkstrand
>Assignee: Radu Cotescu
>Priority: Major
>
> Though it is bad practice, it is possible that an object can return null from
> its toString() method. ObjectModel.toBoolean(Object) \[[Line
> 161|https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/blob/master/src/main/java/org/apache/sling/scripting/sightly/render/ObjectModel.java#L161]\]
> calls .trim() on a potentially null object.
> This causes a difficult to troubleshoot, deeply nested, and cryptic exception
> to be raised. If object.toString() returns null, then it should be treated
> the same as nearly the rest of HTL, where null is considered "falsey". Doing
> so will save hours of difficult troubleshooting.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-8706) Injections for java.util.Optional<> should be automatic optional
[
https://issues.apache.org/jira/browse/SLING-8706?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17142358#comment-17142358
]
Paul Bjorkstrand commented on SLING-8706:
-
I agree with [~justinedelson] on the HTL side. Just because Sling Models
supports this feature doesn't make it required to be supported in HTL (though,
only one supporting with the other not supporting {{Optional}} could lead to
some confusion). I also have never seen any (Java) serialization of Sling
Models, so I don't see that as a barrier either. Besides, serialization [can be
customized|https://howtodoinjava.com/java/serialization/custom-serialization-readobject-writeobject/]
([in more than one
way|https://dzone.com/articles/how-to-customize-serialization-in-java-using-the-e])
to work around non-serializable types already.
Based on the intended use of {{Optional}} (to be used at API boundaries aka
method returns), this I feel is a gray area. While a field isn't technically
part of an API, they are injected by some external process. One could argue
that the fields are equivalent to the return values of that external process.
In that argument, it makes sense to capture that intent in the type system
itself, to capture the fact that "this field could be null, so you need to
check it". This alone makes me think this is a reasonable approach to handling
optional injections.
That being said, there is nothing stopping a developer from improperly using an
{{Optional}}, say, by using {{.get()}} without first checking {{.isPresent()}},
in turn causing a NPE. While bad practice, yes, it happens frequently enough
that I almost always have a conversation with my teams on proper {{Optional}}
usage. I still believe this idea is reasonable, but I don't buy into the
"optional will prevent NPEs" argument.
> Injections for java.util.Optional<> should be automatic optional
> -
>
> Key: SLING-8706
> URL: https://issues.apache.org/jira/browse/SLING-8706
> Project: Sling
> Issue Type: Improvement
> Components: Sling Models
>Reporter: Jörg Hoh
>Priority: Major
> Time Spent: 2h 40m
> Remaining Estimate: 0h
>
> The current approach to support optional injections requires to annotate the
> field with {{@Optional}} plus proper handling within the javacode (null
> checks etc), which can be forgotten.
> So instead of
> {code}
> @Inject @Optional
> String fieldname;
> {code}
> it should also be possible to use this
> {code}
> @Inject
> Optional fieldname;
> {code}
> with the very same semantic. But the developer is forced to deal with the
> case that the value is not present.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Created] (SLING-9314) HTL null pointer in ObjectModel.toBoolean(Object) when object.toString() returns null
Paul Bjorkstrand created SLING-9314: --- Summary: HTL null pointer in ObjectModel.toBoolean(Object) when object.toString() returns null Key: SLING-9314 URL: https://issues.apache.org/jira/browse/SLING-9314 Project: Sling Issue Type: Bug Components: Scripting Affects Versions: Scripting HTL Runtime 1.1.2-1.4.0, Scripting HTL Runtime 1.1.0-1.4.0, Scripting HTL Runtime 1.0.0-1.4.0 Reporter: Paul Bjorkstrand Though it is bad practice, it is possible that an object can return null from its toString() method. ObjectModel.toBoolean(Object) \[[Line 161|https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/blob/master/src/main/java/org/apache/sling/scripting/sightly/render/ObjectModel.java#L161]\] calls .trim() on a potentially null object. This causes a difficult to troubleshoot, deeply nested, and cryptic exception to be raised. If object.toString() returns null, then it should be treated the same as nearly the rest of HTL, where null is considered "falsey". Doing so will save hours of difficult troubleshooting. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (SLING-8425) NPE in SlingScriptEngineManager when Sling is run on GraalVM
[
https://issues.apache.org/jira/browse/SLING-8425?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16846018#comment-16846018
]
Paul Bjorkstrand commented on SLING-8425:
-
Thanks for the assist [~radu.cotescu]. I can confirm that the latest fixes work
on GraalVM.
> NPE in SlingScriptEngineManager when Sling is run on GraalVM
>
>
> Key: SLING-8425
> URL: https://issues.apache.org/jira/browse/SLING-8425
> Project: Sling
> Issue Type: Bug
> Components: Scripting
>Affects Versions: Scripting Core 2.0.54
> Environment: OS: Ubuntu 18.04.2 LTS
> JVM: OpenJDK GraalVM CE 19.0.0 (build 25.212-b03-jvmci-19-b01, mixed mode)
>Reporter: Paul Bjorkstrand
>Assignee: Radu Cotescu
>Priority: Major
> Fix For: Scripting Core 2.0.58
>
> Time Spent: 1.5h
> Remaining Estimate: 0h
>
> When trying to run Sling Starter 11 on GraalVM, there was an NPE in the
> SlingScriptEngineManager when it tried to call
> {{internalManager.registerEngineExtension}} inside {{registerAssociations}}.
> The problem is that a script engine provided by Graal (Nashorn, in this case)
> had {{null}} as an extension value.
> I imagine that it is a bug with GraalVM itself (I have not dug further into
> it yet), but Sling can be defensive, and not call the method(s) inside
> {{registerAssociations}} when it sees a null value for either an extension,
> mime type, or name.
> Fixing this issue also exposes another issue: the
> SlingScriptEngineManagerTest assumes that the JDK it is running on only has a
> single built-in scripting engine. In Graal, there could be two (or more)
> built-in scripting engines. In my situation, there were two: GraalJS and
> Nashorn. Even though Nashorn is a seemingly-broken engine in Graal, it still
> runs through the registration process, so the tests need to account for it.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (SLING-8425) NPE in SlingScriptEngineManager when Sling is run on GraalVM
[
https://issues.apache.org/jira/browse/SLING-8425?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16844221#comment-16844221
]
Paul Bjorkstrand commented on SLING-8425:
-
I found it equally weird. I opened
[https://github.com/oracle/graal/issues/1310] which I hope will get some
attention. If I had to guess why it is this way, when they implemented GraalJS,
instead of modifying Nashorn extensively to remove its reported properties'
values (extensions, mime types, and names), they "cheated" and null-fill an
array somewhere in Nashorn at runtime.
I'll try to keep an eye on that ticket for any movement. If I don't hear back
in a reasonable amount of time (any thoughts of what would be "reasonable"
would be appreciated), would it make sense to move this forward and protect
Sling from the choices of JDK implementors?
> NPE in SlingScriptEngineManager when Sling is run on GraalVM
>
>
> Key: SLING-8425
> URL: https://issues.apache.org/jira/browse/SLING-8425
> Project: Sling
> Issue Type: Bug
> Components: Scripting
>Affects Versions: Scripting Core 2.0.54
> Environment: OS: Ubuntu 18.04.2 LTS
> JVM: OpenJDK GraalVM CE 19.0.0 (build 25.212-b03-jvmci-19-b01, mixed mode)
>Reporter: Paul Bjorkstrand
>Assignee: Radu Cotescu
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> When trying to run Sling Starter 11 on GraalVM, there was an NPE in the
> SlingScriptEngineManager when it tried to call
> {{internalManager.registerEngineExtension}} inside {{registerAssociations}}.
> The problem is that a script engine provided by Graal (Nashorn, in this case)
> had {{null}} as an extension value.
> I imagine that it is a bug with GraalVM itself (I have not dug further into
> it yet), but Sling can be defensive, and not call the method(s) inside
> {{registerAssociations}} when it sees a null value for either an extension,
> mime type, or name.
> Fixing this issue also exposes another issue: the
> SlingScriptEngineManagerTest assumes that the JDK it is running on only has a
> single built-in scripting engine. In Graal, there could be two (or more)
> built-in scripting engines. In my situation, there were two: GraalJS and
> Nashorn. Even though Nashorn is a seemingly-broken engine in Graal, it still
> runs through the registration process, so the tests need to account for it.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Created] (SLING-8425) NPE in SlingScriptEngineManager when Sling is run on GraalVM
Paul Bjorkstrand created SLING-8425:
---
Summary: NPE in SlingScriptEngineManager when Sling is run on
GraalVM
Key: SLING-8425
URL: https://issues.apache.org/jira/browse/SLING-8425
Project: Sling
Issue Type: Bug
Components: Scripting
Affects Versions: Scripting Core 2.0.54
Environment: OS: Ubuntu 18.04.2 LTS
JVM: OpenJDK GraalVM CE 19.0.0 (build 25.212-b03-jvmci-19-b01, mixed mode)
Reporter: Paul Bjorkstrand
When trying to run Sling Starter 11 on GraalVM, there was an NPE in the
SlingScriptEngineManager when it tried to call
{{internalManager.registerEngineExtension}} inside {{registerAssociations}}.
The problem is that a script engine provided by Graal (Nashorn, in this case)
had {{null}} as an extension value.
I imagine that it is a bug with GraalVM itself (I have not dug further into it
yet), but Sling can be defensive, and not call the method(s) inside
{{registerAssociations}} when it sees a null value for either an extension,
mime type, or name.
Fixing this issue also exposes another issue: the SlingScriptEngineManagerTest
assumes that the JDK it is running on only has a single built-in scripting
engine. In Graal, there could be two (or more) built-in scripting engines. In
my situation, there were two: GraalJS and Nashorn. Even though Nashorn is a
seemingly-broken engine in Graal, it still runs through the registration
process, so the tests need to account for it.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (SLING-8425) NPE in SlingScriptEngineManager when Sling is run on GraalVM
[
https://issues.apache.org/jira/browse/SLING-8425?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16844047#comment-16844047
]
Paul Bjorkstrand commented on SLING-8425:
-
I have a proposed fix in the works, and will submit a PR later today.
> NPE in SlingScriptEngineManager when Sling is run on GraalVM
>
>
> Key: SLING-8425
> URL: https://issues.apache.org/jira/browse/SLING-8425
> Project: Sling
> Issue Type: Bug
> Components: Scripting
>Affects Versions: Scripting Core 2.0.54
> Environment: OS: Ubuntu 18.04.2 LTS
> JVM: OpenJDK GraalVM CE 19.0.0 (build 25.212-b03-jvmci-19-b01, mixed mode)
>Reporter: Paul Bjorkstrand
>Priority: Major
>
> When trying to run Sling Starter 11 on GraalVM, there was an NPE in the
> SlingScriptEngineManager when it tried to call
> {{internalManager.registerEngineExtension}} inside {{registerAssociations}}.
> The problem is that a script engine provided by Graal (Nashorn, in this case)
> had {{null}} as an extension value.
> I imagine that it is a bug with GraalVM itself (I have not dug further into
> it yet), but Sling can be defensive, and not call the method(s) inside
> {{registerAssociations}} when it sees a null value for either an extension,
> mime type, or name.
> Fixing this issue also exposes another issue: the
> SlingScriptEngineManagerTest assumes that the JDK it is running on only has a
> single built-in scripting engine. In Graal, there could be two (or more)
> built-in scripting engines. In my situation, there were two: GraalJS and
> Nashorn. Even though Nashorn is a seemingly-broken engine in Graal, it still
> runs through the registration process, so the tests need to account for it.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (SLING-8069) Sling Models: Enable constructor injection to use non-public constructors
[
https://issues.apache.org/jira/browse/SLING-8069?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16673606#comment-16673606
]
Paul Bjorkstrand commented on SLING-8069:
-
Added Github PR link since it didn't link automatically
> Sling Models: Enable constructor injection to use non-public constructors
> -
>
> Key: SLING-8069
> URL: https://issues.apache.org/jira/browse/SLING-8069
> Project: Sling
> Issue Type: Improvement
>Affects Versions: Sling Models Impl 1.4.10
>Reporter: Paul Bjorkstrand
>Priority: Minor
>
> In Sling Models, you cannot use a non-public constructor to inject. Looking
> through the code, there doesn't seem to be any clear reason for this
> restriction. In my opinion, constructor injection should allow any
> constructor visibility.
> Here are some discussion points:
> * Private fields are allowed for use, so disallowing private constructors
> seems unnecessary.
> * Private constructors may be bad practice (difficult to test), but Sling
> should not be telling users how to write their Java code. This is especially
> true for models, since it should work with POJOs, as stated in the
> documentation. It would be trivial to add checks to just allow default,
> protected, or public, but I feel that logic is unnecessary.
> * Non-public methods could also be allowed, but that can be a separate
> ticket.
> ** A prerequisite of this would be to allow setter injection on models in
> the first place. Again, not the subject of this ticket.
> * Threading concerns are minimal, but there could be possible deadlocks, as
> with any multi-threaded application that uses locks. In general, I think
> locking similar to how it is done in {{InjectableField}} would be sufficient.
> The risk of deadlock would be similar to the risk of the locking in
> {{injectableField.set(Object, Result)}}.
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Created] (SLING-8069) Sling Models: Enable constructor injection to use non-public constructors
Paul Bjorkstrand created SLING-8069:
---
Summary: Sling Models: Enable constructor injection to use
non-public constructors
Key: SLING-8069
URL: https://issues.apache.org/jira/browse/SLING-8069
Project: Sling
Issue Type: Improvement
Affects Versions: Sling Models Impl 1.4.10
Reporter: Paul Bjorkstrand
In Sling Models, you cannot use a non-public constructor to inject. Looking
through the code, there doesn't seem to be any clear reason for this
restriction. In my opinion, constructor injection should allow any constructor
visibility.
Here are some discussion points:
* Private fields are allowed for use, so disallowing private constructors
seems unnecessary.
* Private constructors may be bad practice (difficult to test), but Sling
should not be telling users how to write their Java code. This is especially
true for models, since it should work with POJOs, as stated in the
documentation. It would be trivial to add checks to just allow default,
protected, or public, but I feel that logic is unnecessary.
* Non-public methods could also be allowed, but that can be a separate ticket.
** A prerequisite of this would be to allow setter injection on models in the
first place. Again, not the subject of this ticket.
* Threading concerns are minimal, but there could be possible deadlocks, as
with any multi-threaded application that uses locks. In general, I think
locking similar to how it is done in {{InjectableField}} would be sufficient.
The risk of deadlock would be similar to the risk of the locking in
{{injectableField.set(Object, Result)}}.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Created] (SLING-7549) tag is not being treated as "self closing" when used in conjunction with HTL block statements
Paul Bjorkstrand created SLING-7549:
---
Summary: tag is not being treated as "self closing" when
used in conjunction with HTL block statements
Key: SLING-7549
URL: https://issues.apache.org/jira/browse/SLING-7549
Project: Sling
Issue Type: Bug
Components: Scripting
Affects Versions: Scripting HTL Engine 1.0.32
Reporter: Paul Bjorkstrand
When using HTL block statements some tags that are assumed to be "self closing"
by browsers are not handled in that manner by the HTL engine. This can cause
hard to find problems with the rendering of the page. in this example, the
{{}} tag was the culprit.
I was diagnosing an issue where under a certain set of circumstances all of my
page's css was not being rendered. This happened to be due a {{}} tag
that was not closed, but had a block statement added to it:
{code:java}
{code}
That seemingly innocuous tag caused the HTL engine to assume that all contents
following, (until the the parent head closing tag), to be inside that link tag.
According to the [html5
spec|https://www.w3.org/TR/2014/REC-html5-20141028/document-metadata.html#the-link-element]
the content of the {{}} element is empty. Nearly every browser in use
today treats {{}} the same as {{}}. Given this, it is not
a stretch to assume that HTL would treat it in the same manner.
It is my belief that HTL should mimic the html5 specification. At minimum, I
would hope to get a clear answer on which HTML specification it follows, or a
statement that HTL does not/will not follow an HTML specification.
*Note*: I was unsure if this should be something in the HTL spec or in the
implementation. Since I found no reference in the specification regarding
html5/xhtml/html4 compliance, I came here with my request instead. Please let
me know if this is something that should be addressed at the specification
level.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
