[jira] [Commented] (SLING-11507) Field injection should not inject static fields
[
https://issues.apache.org/jira/browse/SLING-11507?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17630447#comment-17630447
]
Stefan Seifert commented on SLING-11507:
i've looked at the PR
https://github.com/apache/sling-org-apache-sling-models-api/pull/8 - lgtm!
it would also propose to fail on final fields. final fields should only be used
with constructor injection, when the field itself is managed by the model code.
> Field injection should not inject static fields
> ---
>
> Key: SLING-11507
> URL: https://issues.apache.org/jira/browse/SLING-11507
> Project: Sling
> Issue Type: Bug
> Components: Sling Models
>Affects Versions: Models Impl 1.4.14
>Reporter: Joerg Hoh
>Assignee: Dirk Rudolph
>Priority: Critical
> Time Spent: 1h 10m
> Remaining Estimate: 0h
>
> Currently this injection works:
> {noformat}
> @SlingObject
> private static ResourceResolver resourceResolver;
> {noformat}
> but it should not. Sling Models are Pojos and injection must never inject
> into static fields. Instead it should throw an exception and an error message
> indicating the problem (another option would be to refuse the injection with
> a proper error message).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-11507) Field injection should not inject static fields
[
https://issues.apache.org/jira/browse/SLING-11507?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17630380#comment-17630380
]
Dirk Rudolph commented on SLING-11507:
--
I am wondering, if we also should fail for injections of final fields. I know
that this caused issues with injections done by the OSGI declarative service
runtime in the past.
Thoughts?
> Field injection should not inject static fields
> ---
>
> Key: SLING-11507
> URL: https://issues.apache.org/jira/browse/SLING-11507
> Project: Sling
> Issue Type: Bug
> Components: Sling Models
>Affects Versions: Models Impl 1.4.14
>Reporter: Joerg Hoh
>Priority: Critical
> Fix For: Sling Models Implementation 1.5
>
> Time Spent: 50m
> Remaining Estimate: 0h
>
> Currently this injection works:
> {noformat}
> @SlingObject
> private static ResourceResolver resourceResolver;
> {noformat}
> but it should not. Sling Models are Pojos and injection must never inject
> into static fields. Instead it should throw an exception and an error message
> indicating the problem (another option would be to refuse the injection with
> a proper error message).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-11507) Field injection should not inject static fields
[
https://issues.apache.org/jira/browse/SLING-11507?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17630363#comment-17630363
]
Robert Munteanu commented on SLING-11507:
-
Thanks for taking this up [~diru]!
> Field injection should not inject static fields
> ---
>
> Key: SLING-11507
> URL: https://issues.apache.org/jira/browse/SLING-11507
> Project: Sling
> Issue Type: Bug
> Components: Sling Models
>Affects Versions: Models Impl 1.4.14
>Reporter: Joerg Hoh
>Priority: Critical
> Fix For: Sling Models Implementation 1.5
>
> Time Spent: 50m
> Remaining Estimate: 0h
>
> Currently this injection works:
> {noformat}
> @SlingObject
> private static ResourceResolver resourceResolver;
> {noformat}
> but it should not. Sling Models are Pojos and injection must never inject
> into static fields. Instead it should throw an exception and an error message
> indicating the problem (another option would be to refuse the injection with
> a proper error message).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-11507) Field injection should not inject static fields
[
https://issues.apache.org/jira/browse/SLING-11507?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17630317#comment-17630317
]
Angela Schreiber commented on SLING-11507:
--
[~diru], thanks a lot! your post and me pinging [~sseifert] and [~kwin] have
just crossed. i am not familiar with sling models but if a review/input from an
outsider is helpful please ping me.
> Field injection should not inject static fields
> ---
>
> Key: SLING-11507
> URL: https://issues.apache.org/jira/browse/SLING-11507
> Project: Sling
> Issue Type: Bug
> Components: Sling Models
>Affects Versions: Models Impl 1.4.14
>Reporter: Joerg Hoh
>Priority: Critical
> Fix For: Sling Models Implementation 1.5
>
> Time Spent: 40m
> Remaining Estimate: 0h
>
> Currently this injection works:
> {noformat}
> @SlingObject
> private static ResourceResolver resourceResolver;
> {noformat}
> but it should not. Sling Models are Pojos and injection must never inject
> into static fields. Instead it should throw an exception and an error message
> indicating the problem (another option would be to refuse the injection with
> a proper error message).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-11507) Field injection should not inject static fields
[
https://issues.apache.org/jira/browse/SLING-11507?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17630313#comment-17630313
]
Angela Schreiber commented on SLING-11507:
--
hi [~sseifert] and [~kwin], it would be awesome if you could provide input or
help to get this issue fixed. wdyt?
cc: [~cziegeler], [~chaotic]
> Field injection should not inject static fields
> ---
>
> Key: SLING-11507
> URL: https://issues.apache.org/jira/browse/SLING-11507
> Project: Sling
> Issue Type: Bug
> Components: Sling Models
>Affects Versions: Models Impl 1.4.14
>Reporter: Joerg Hoh
>Priority: Critical
> Fix For: Sling Models Implementation 1.5
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Currently this injection works:
> {noformat}
> @SlingObject
> private static ResourceResolver resourceResolver;
> {noformat}
> but it should not. Sling Models are Pojos and injection must never inject
> into static fields. Instead it should throw an exception and an error message
> indicating the problem (another option would be to refuse the injection with
> a proper error message).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-11507) Field injection should not inject static fields
[
https://issues.apache.org/jira/browse/SLING-11507?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17630312#comment-17630312
]
Dirk Rudolph commented on SLING-11507:
--
I don't know the full history of this either, but agree that this may have been
overlooked in the past.
I can take this up and continue on what [~rombert] already started, first
adding some ITs using the maven-invoker-plugin and adapting the annotation
processor to handle all cases.
I already added a first IT for reference in
https://github.com/apache/sling-org-apache-sling-models-api/pull/8/commits/8f1f05f69b4de2ad510c335a2e939de2c2fbb133
> Field injection should not inject static fields
> ---
>
> Key: SLING-11507
> URL: https://issues.apache.org/jira/browse/SLING-11507
> Project: Sling
> Issue Type: Bug
> Components: Sling Models
>Affects Versions: Models Impl 1.4.14
>Reporter: Joerg Hoh
>Priority: Critical
> Fix For: Sling Models Implementation 1.5
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Currently this injection works:
> {noformat}
> @SlingObject
> private static ResourceResolver resourceResolver;
> {noformat}
> but it should not. Sling Models are Pojos and injection must never inject
> into static fields. Instead it should throw an exception and an error message
> indicating the problem (another option would be to refuse the injection with
> a proper error message).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-11507) Field injection should not inject static fields
[
https://issues.apache.org/jira/browse/SLING-11507?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17628967#comment-17628967
]
Robert Munteanu commented on SLING-11507:
-
[~angela] - looking at the contributors list (
https://github.com/apache/sling-org-apache-sling-models-api/graphs/contributors
,
https://github.com/apache/sling-org-apache-sling-models-impl/graphs/contributors
) I see that [~sseifert], [~kwin], and [~diru] are the most "prolific"
contributors.
Others did contribute, but either occasionally or for "technical" issues -
readmes, parent pom, Jenkins, etc.
> Field injection should not inject static fields
> ---
>
> Key: SLING-11507
> URL: https://issues.apache.org/jira/browse/SLING-11507
> Project: Sling
> Issue Type: Bug
> Components: Sling Models
>Affects Versions: Models Impl 1.4.14
>Reporter: Joerg Hoh
>Priority: Critical
> Fix For: Sling Models Implementation 1.5
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Currently this injection works:
> {noformat}
> @SlingObject
> private static ResourceResolver resourceResolver;
> {noformat}
> but it should not. Sling Models are Pojos and injection must never inject
> into static fields. Instead it should throw an exception and an error message
> indicating the problem (another option would be to refuse the injection with
> a proper error message).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-11507) Field injection should not inject static fields
[
https://issues.apache.org/jira/browse/SLING-11507?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17628946#comment-17628946
]
Angela Schreiber commented on SLING-11507:
--
[~rombert], sounds good to me.
IMHO having static fields/methods annotated sounds like a terrible idea and I
can't imagine that this was ever intended to work but rather simply was
overlooked.
So, I think we should actually treat this as a bug and not just as a task or
improvement. Would you know who is the original author or otherwise deeper
knowledge into this area and could provide additional insight and possible work
on a fix?
> Field injection should not inject static fields
> ---
>
> Key: SLING-11507
> URL: https://issues.apache.org/jira/browse/SLING-11507
> Project: Sling
> Issue Type: Bug
> Components: Sling Models
>Affects Versions: Models Impl 1.4.14
>Reporter: Joerg Hoh
>Priority: Critical
> Fix For: Sling Models Implementation 1.5
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Currently this injection works:
> {noformat}
> @SlingObject
> private static ResourceResolver resourceResolver;
> {noformat}
> but it should not. Sling Models are Pojos and injection must never inject
> into static fields. Instead it should throw an exception and an error message
> indicating the problem (another option would be to refuse the injection with
> a proper error message).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-11507) Field injection should not inject static fields
[
https://issues.apache.org/jira/browse/SLING-11507?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17628697#comment-17628697
]
Carsten Ziegeler commented on SLING-11507:
--
[~rombert] That approach sounds right to me
> Field injection should not inject static fields
> ---
>
> Key: SLING-11507
> URL: https://issues.apache.org/jira/browse/SLING-11507
> Project: Sling
> Issue Type: Task
> Components: Sling Models
>Affects Versions: Models Impl 1.4.14
>Reporter: Joerg Hoh
>Priority: Major
> Fix For: Sling Models Implementation 1.5
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Currently this injection works:
> {noformat}
> @SlingObject
> private static ResourceResolver resourceResolver;
> {noformat}
> but it should not. Sling Models are Pojos and injection must never inject
> into static fields. Instead it should throw an exception and an error message
> indicating the problem (another option would be to refuse the injection with
> a proper error message).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-11507) Field injection should not inject static fields
[
https://issues.apache.org/jira/browse/SLING-11507?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17628266#comment-17628266
]
Robert Munteanu commented on SLING-11507:
-
Ok, so the approach seems to be something like:
- scan for classes annotated with "Model"
- for each class, check if static fields or static methods are annotated with
any annotation supported by Sling models
- if any such field/method is found, error out
It should be possible to set up a couple of smoke tests using the
maven-invoker-plugin and we should be good to go.
I won't have time to "productise" this in the short term unfortunately
> Field injection should not inject static fields
> ---
>
> Key: SLING-11507
> URL: https://issues.apache.org/jira/browse/SLING-11507
> Project: Sling
> Issue Type: Task
> Components: Sling Models
>Affects Versions: Models Impl 1.4.14
>Reporter: Joerg Hoh
>Priority: Major
> Fix For: Sling Models Implementation 1.5
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Currently this injection works:
> {noformat}
> @SlingObject
> private static ResourceResolver resourceResolver;
> {noformat}
> but it should not. Sling Models are Pojos and injection must never inject
> into static fields. Instead it should throw an exception and an error message
> indicating the problem (another option would be to refuse the injection with
> a proper error message).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-11507) Field injection should not inject static fields
[
https://issues.apache.org/jira/browse/SLING-11507?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17627496#comment-17627496
]
Carsten Ziegeler commented on SLING-11507:
--
[~rombert] True, a static method could be written in a thread-safe way, however
- unless I'm overlooking something - the methods never get the instance passed
in as an argument. So the only thing a static method could do is tell the world
that it got invoked or potentially setting up a shared resource for the
instances when invoked for the first time. However, these look like
anti-patterns to me as well.
For annotation scanning, I think we need to scan for both "inject annotations".
Not sure, if the class needs to be annotated with "Model" or whether you can
pass in any class into the machinery?
> Field injection should not inject static fields
> ---
>
> Key: SLING-11507
> URL: https://issues.apache.org/jira/browse/SLING-11507
> Project: Sling
> Issue Type: Task
> Components: Sling Models
>Affects Versions: Models Impl 1.4.14
>Reporter: Joerg Hoh
>Priority: Major
> Fix For: Sling Models Implementation 1.5
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Currently this injection works:
> {noformat}
> @SlingObject
> private static ResourceResolver resourceResolver;
> {noformat}
> but it should not. Sling Models are Pojos and injection must never inject
> into static fields. Instead it should throw an exception and an error message
> indicating the problem (another option would be to refuse the injection with
> a proper error message).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-11507) Field injection should not inject static fields
[
https://issues.apache.org/jira/browse/SLING-11507?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17627018#comment-17627018
]
Robert Munteanu commented on SLING-11507:
-
I can definitely extend this to static methods. It is less clear to me though
if we should do that though, as the method may be written in a thread-safe way.
It's also not clear to me if we should check for other annotations like
{{@Inject}}, which increases the chance to conflict with other libraries.
Should we check that the top-level class is annotated with with {{@Model}} ?
> Field injection should not inject static fields
> ---
>
> Key: SLING-11507
> URL: https://issues.apache.org/jira/browse/SLING-11507
> Project: Sling
> Issue Type: Task
> Components: Sling Models
>Affects Versions: Models Impl 1.4.14
>Reporter: Joerg Hoh
>Priority: Major
> Fix For: Sling Models Implementation 1.5
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Currently this injection works:
> {noformat}
> @SlingObject
> private static ResourceResolver resourceResolver;
> {noformat}
> but it should not. Sling Models are Pojos and injection must never inject
> into static fields. Instead it should throw an exception and an error message
> indicating the problem (another option would be to refuse the injection with
> a proper error message).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-11507) Field injection should not inject static fields
[
https://issues.apache.org/jira/browse/SLING-11507?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17626679#comment-17626679
]
Carsten Ziegeler commented on SLING-11507:
--
I was wondering if we should extend this to methods as well. I think it doesn't
make sense to call static methods either.
> Field injection should not inject static fields
> ---
>
> Key: SLING-11507
> URL: https://issues.apache.org/jira/browse/SLING-11507
> Project: Sling
> Issue Type: Task
> Components: Sling Models
>Affects Versions: Models Impl 1.4.14
>Reporter: Joerg Hoh
>Priority: Major
> Fix For: Sling Models Implementation 1.5
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> Currently this injection works:
> {noformat}
> @SlingObject
> private static ResourceResolver resourceResolver;
> {noformat}
> but it should not. Sling Models are Pojos and injection must never inject
> into static fields. Instead it should throw an exception and an error message
> indicating the problem (another option would be to refuse the injection with
> a proper error message).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-11507) Field injection should not inject static fields
[
https://issues.apache.org/jira/browse/SLING-11507?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17626674#comment-17626674
]
Robert Munteanu commented on SLING-11507:
-
Created a Draft PR that shows how this can look -
https://github.com/apache/sling-org-apache-sling-models-api/pull/8 . Still
draft since I would like to add some automated tests and ensure that we catch
all the fault scenarios.
> Field injection should not inject static fields
> ---
>
> Key: SLING-11507
> URL: https://issues.apache.org/jira/browse/SLING-11507
> Project: Sling
> Issue Type: Task
> Components: Sling Models
>Affects Versions: Models Impl 1.4.14
>Reporter: Joerg Hoh
>Priority: Major
> Fix For: Sling Models Implementation 1.5
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Currently this injection works:
> {noformat}
> @SlingObject
> private static ResourceResolver resourceResolver;
> {noformat}
> but it should not. Sling Models are Pojos and injection must never inject
> into static fields. Instead it should throw an exception and an error message
> indicating the problem (another option would be to refuse the injection with
> a proper error message).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-11507) Field injection should not inject static fields
[
https://issues.apache.org/jira/browse/SLING-11507?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17626410#comment-17626410
]
Carsten Ziegeler commented on SLING-11507:
--
Unfortunately, just looking at the header is not enough . it is possible to use
any class as a Sling Model through API without declaring it in a manifest
header.
So, I think ultimately we need to scan all classes for the inject annotation -
but that is actually simplifying it as we can just blindly go through all
classes.
> Field injection should not inject static fields
> ---
>
> Key: SLING-11507
> URL: https://issues.apache.org/jira/browse/SLING-11507
> Project: Sling
> Issue Type: Task
> Components: Sling Models
>Affects Versions: Models Impl 1.4.14
>Reporter: Joerg Hoh
>Priority: Major
> Fix For: Sling Models Implementation 1.5
>
>
> Currently this injection works:
> {noformat}
> @SlingObject
> private static ResourceResolver resourceResolver;
> {noformat}
> but it should not. Sling Models are Pojos and injection must never inject
> into static fields. Instead it should throw an exception and an error message
> indicating the problem (another option would be to refuse the injection with
> a proper error message).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-11507) Field injection should not inject static fields
[
https://issues.apache.org/jira/browse/SLING-11507?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17625766#comment-17625766
]
Robert Munteanu commented on SLING-11507:
-
What is the tooling entry point that we would have for a build-time failures?
Looking at [the Sling Models
documentation|https://sling.apache.org/documentation/bundles/models.html] it
seems to me all of the processing is done at runtime. There is a potential
exception for adding the {{Sling-Model-Packages}} header to the OSGi bundle,
which can be done manually anyway.
There is a bnd plugin we are using where the check could be added -
https://github.com/apache/sling-org-apache-sling-bnd-models , but that only
applies to projects using bnd and this plug-in.
Another build-time option is an annotation processor, which would trigger a
compilation error when an Sling models annotation is found on a static field.
To my knowledge, it should be invoked automatically by Maven. The only open
question that I have is whether we can detect the {{Sling-Model-Packages}}
header so that we only apply the check to configured classes.
> Field injection should not inject static fields
> ---
>
> Key: SLING-11507
> URL: https://issues.apache.org/jira/browse/SLING-11507
> Project: Sling
> Issue Type: Task
> Components: Sling Models
>Affects Versions: Models Impl 1.4.14
>Reporter: Joerg Hoh
>Priority: Major
> Fix For: Sling Models Implementation 1.5
>
>
> Currently this injection works:
> {noformat}
> @SlingObject
> private static ResourceResolver resourceResolver;
> {noformat}
> but it should not. Sling Models are Pojos and injection must never inject
> into static fields. Instead it should throw an exception and an error message
> indicating the problem (another option would be to refuse the injection with
> a proper error message).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-11507) Field injection should not inject static fields
[
https://issues.apache.org/jira/browse/SLING-11507?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17597070#comment-17597070
]
Joerg Hoh commented on SLING-11507:
---
[~cziegeler] Valid point. We should always print a warning and just optionally
refuse the injection.
> Field injection should not inject static fields
> ---
>
> Key: SLING-11507
> URL: https://issues.apache.org/jira/browse/SLING-11507
> Project: Sling
> Issue Type: Task
> Components: Sling Models
>Affects Versions: Models Impl 1.4.14
>Reporter: Joerg Hoh
>Priority: Major
>
> Currently this injection works:
> {noformat}
> @SlingObject
> private static ResourceResolver resourceResolver;
> {noformat}
> but it should not. Sling Models are Pojos and injection must never inject
> into static fields. Instead it should throw an exception and an error message
> indicating the problem.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-11507) Field injection should not inject static fields
[
https://issues.apache.org/jira/browse/SLING-11507?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17597025#comment-17597025
]
Carsten Ziegeler commented on SLING-11507:
--
This is clearly a developer error and should be prevented. The only problem I
see is that the check today happens at runtime (unless I'm overlooking
something). So if we switch to prevent this, existing bad code might break
unexpectedly at runtime. It would be good if we could have a build error
instead.
> Field injection should not inject static fields
> ---
>
> Key: SLING-11507
> URL: https://issues.apache.org/jira/browse/SLING-11507
> Project: Sling
> Issue Type: Task
> Components: Sling Models
>Affects Versions: Models Impl 1.4.14
>Reporter: Joerg Hoh
>Priority: Major
>
> Currently this injection works:
> {noformat}
> @SlingObject
> private static ResourceResolver resourceResolver;
> {noformat}
> but it should not. Sling Models are Pojos and injection must never inject
> into static fields. Instead it should throw an exception and an error message
> indicating the problem.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
