SLING-8843 repoinit developer mode

[joerghoh]

when providing the system property
"org.apache.sling.jcr.repoinit.developermode" with the value "true"
repoinit will not throw exception and terminate the startup of the
repository, but instead logs an message pointing this out.
Thus it's easier for a developer to fix the script without the need to
make the repository going first.

To clearly point out potential mis-use (having it configured in
production environments), this log message is written on loglevel ERROR
and also the on startup the "developer mode active" INFO message is
written.

[enapps-enorman]

I'm not necessarily opposed to this, but perhaps it would be more useful to introduce a generalized solution to alter behavior in production vs development.

For example, the JavaServerFaces framework has a ProjectStage concept that can be checked at runtime to alter behaviors.

Sling could do something similar with a configurable ProjectStage service that could be referenced wherever this kind of decision is required.

[bdelacretaz]

Note that we do have a simple way to test log messages, at https://github.com/apache/sling-org-apache-sling-graphql-core/blob/master/src/test/java/org/apache/sling/graphql/core/util/LogCapture.java - this would allow more precise testing (and we might want to move that class to a common module, but it is reusable already for tests)

[rombert]

LGTM overall. Not an expert in repoinit, so I just commented on some things that stood out to me.

[joerghoh]

@enapps-enorman For a more generalized behavior we do have the runmodes.

I decided against using OSGI configuration (and thus the ability to configure it per runmode) because this should be development only setting. If it is OSGI it would be a default setting in every project, which might end up in production.
OSGI configuration is normally deployed along code, but system properties are not. Makes it harder to misuse.

[enapps-enorman]

Makes it harder to misuse.

@joerghoh To me the distinction you have made seems to be purely semantics. If the ProjectStage configuration defaults to "Production" then your code would only do the "Development" stuff when enabled manually. For example, if the user starts the feature launcher with a command line property variable that changes the default value or the admin manually changes the value using the ConfigurationAdmin.

I think I would still prefer a generalized solution since the same decision could be useful to improve the sling DefaultErrorHandler to not return stacktrace or request progress details when in production. Exposing those implementation details of the server to the end users is a potential information disclosure vulnerability.

Another example is perhaps you want to send minimized js files in production and non-minimized during development.

[joerghoh]

@enapps-enorman For the environment-specific settings we have runmodes, I don't see the real need or value to introduce a new concept here.

For the moment I want to leave this setting as a system property. If there's real demand for it, we still can convert it into an OSGI setting.

[enapps-enorman]

If there's real demand for it,

Is my demand for it not real?

[joerghoh]

I would then change it to use OSGI configuration, and not introducing a new abstraction for it. With runmodes we have the same capabilities, when it comes to support different settings.

@enapps-enorman I consider this as an emergency setting, so developers do not have to rebuild their local environment, when they made a mistake with repoinit scripts. If this is a system property they can fix their environment even if it's not coming up anymore because of an incorrect repoinit script. With OSGI settings this gets a bit more complex; and not every developer starts its application with a feature launcher.

[enapps-enorman]

I consider this as an emergency setting

Then don't call it "developer mode", call it something else.

Perhaps something more descriptive like "org.apache.sling.jcr.repoinit.keepRunningOnError".

[bdelacretaz]

+1 for a more descriptive name for the property.

By analogy with Maven's failIfNoTests setting I suggest org.apache.sling.repoinit.failOnError which defaults to true.

[bdelacretaz]

"set to non-true" takes more mental bandwidth than "false" or "set to false", IMHO ;-)

[bdelacretaz]

nitpick, now that the property is named "fail on error" I'd use the same terminology here and call the method failOnError

[sonarqubecloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

65.0% Coverage
0.0% Duplication

[kwin]

isn't slf4j-simple enough for the tests?

[kwin]

Let us please use the OSGi way in order to allow using framework properties as well: https://docs.osgi.org/javadoc/r4v43/core/org/osgi/framework/BundleContext.html#getProperty(java.lang.String)