[JENKINS-60866][JENKINS-71515] Use JSON#parse to process codemirror-config argument

[daniel-beck]

See JENKINS-60866.

jenkinsci/antisamy-markup-formatter-plugin#91 would be nice to have, but not necessary.

Strictly speaking, this "just" makes the code behave as documented at

jenkins/core/src/main/resources/lib/form/textarea.jelly

Lines 68 to 71 in d977ee8

	<st:attribute name="codemirror-config">
	Specifies additional key/value pairs in the JSON format (except the start and end bracket)
	to be passed as CodeMirror option object.
	</st:attribute>

but since tons of plugins set this value badly, I added backward compatibility for what appears to be the most common configuration (mode).

Proposed changelog entries

• Require syntactically valid JSON snippet to be returned from MarkupFormatter#getCodemirrorConfig / provided to codemirror-config in f:textarea.

Proposed upgrade guidelines

N/A

Submitter checklist

• (If applicable) Jira issue is well described
• Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change) and are in the imperative mood. Examples
  • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
• Appropriate autotests or explanation to why this change has no tests
• New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
• New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO") if applicable.
• For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

• There are at least 2 approvals for the pull request and no outstanding requests for change
• Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
• Changelog entries in the PR title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood
• Proper changelog labels are set so that the changelog can be generated automatically
• If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
• If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

[daniel-beck]

I don't really see why I needed jenkinsci/antisamy-markup-formatter-plugin#91 to make the test pass, any ideas?

[github-actions]

Please take a moment and address the merge conflicts of your pull request. Thanks!

[basil]

What are the next steps for this PR? As far as I can tell jenkinsci/antisamy-markup-formatter-plugin#91 needs to be merged and released, then this PR needs to be updated to a non-incremental build of antisamy-markup-formatter-plugin. Is there any additional work that needs to be done?

[daniel-beck]

What are the next steps for this PR? As far as I can tell jenkinsci/antisamy-markup-formatter-plugin#91 needs to be merged and released, then this PR needs to be updated to a non-incremental build of antisamy-markup-formatter-plugin. Is there any additional work that needs to be done?

Ideally I'd find the time to understand why this needs the plugin update despite the attempt at backwards compatibility. HTMLUnit throws at codemirror.js line 2246, and then

jenkins/test/src/test/java/lib/form/ApplyButtonTest.java

Line 38 in 0d1e8e6

config.executeJavaScript("document.getElementsByTagName('TEXTAREA')[0].codemirrorObject.setLine(0,'foobar')");

doesn't work because codeMirrorObject isn't set.

Otherwise this is just waiting for the plugin release, yes.

[basil]

With a passing test suite, this is looking close to ready (if not fully ready) from my perspective. Do others feel that the mode compatibility path is sufficient, or do others feel that a more systematic search of cases not covered by that compatibility path is needed? My personal opinion is that while the latter could not hurt, I leave that to Daniel's discretion and trust his judgment regarding whether a systematic search is necessary or not.

[NotMyFault]

Looks fine so far, maybe we can get a release of the antisamy-markup plugin soonish, but I wouldn't consider this as blocker, given the antisamy PR is up for review.

[basil]

antisamy-markup-formatter-plugin is fully covered by the compatibility path, so while that PR is valuable as a code cleanup, there is no need to hold this PR for that one. Whether there are other plugins not covered by the compatibility path that ought to be considered is a bit of an open question, but as I wrote earlier I won't insist on a systematic search.

[Wadeck]

@NotMyFault I will ask someone (else than Daniel) from security team 👍

[NotMyFault]

@NotMyFault I will ask someone (else than Daniel) from security team 👍

Thanks, just mentioned you, because Daniel requested your review initially.

[Wadeck]

it was because of CSP topic :) but you did right, thanks 👍

[daniel-beck]

Just in case, I don't think this should be merged yet. I haven't done the followup work for #6867 (comment) yet.

So while I think this change is final and can be reviewed, I recommend we hold off merging. IIRC, this will break CodeMirror use in several plugins, most notably job-dsl, groovy, and powershell plugins.

[basil]

jenkinsci/cloudbees-feature-management-plugin#86 only has 40 installations, and everything else is merged and released, so are we ready to move forward with this?

[daniel-beck]

@basil Depends on your threshold for plugin popularity. Based on the notes I took ~2 yrs ago, templating-engine is the only one left with >500 known installs that would be incompatible. I suggest a re-review of current plugins for compatibility in case something new popped up since. I'd be happy to share the old notes or to do it myself, but cannot commit to a date right now for the latter.

[basil]

While I appreciate the desire for perfection here, perhaps we are falling into analysis paralysis and letting "perfect" be the enemy of "good enough." The value of shipping code is far higher than the value of non-shipping code, and the cost of old PRs is high (including the cost of retesting), so I think we should take it out of draft and ship it in weekly (especially since we are early in an LTS cycle). Whatever minor issues remain can be dealt with if and when they come up.

[basil]

Passing ATH: jenkinsci/acceptance-test-harness#1748

[basil]

Passing PCT: jenkinsci/bom#3651

[basil]

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process. Thanks!