mirror of
https://github.com/apache/struts.git
synced 2026-08-05 06:36:58 +00:00
main
17 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Lukasz Lenart
|
a6570b769b |
WW-5668 Bound the localized-text provider caches and align request-locale resolution (#1821)
* WW-5668 docs: design spec for bounded i18n caches and request-locale resolution Follow-up to WW-5540. Bound the AbstractLocalizedTextProvider caches via the existing OgnlCache abstraction (configurable struts.i18n.cacheType/cacheMaxSize), and add opt-in request-locale resolution consistency between Dispatcher and I18nInterceptor (struts.locale.validateRequestLocale, default off). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 docs: implementation plan for bounded i18n caches and request-locale resolution Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Add remove(key) to the OgnlCache abstraction Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Bound the localized-text provider caches with configurable size Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Fix reassignable-lock hazard and add volatile to i18n cache fields synchronized (bundlesMap) locked on a monitor that rebuildI18nCaches() can reassign; introduce a dedicated bundlesMapLock and lock on that instead. Mark the five i18n cache fields volatile for safe publication across the reassignment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Add opt-in request-locale resolution consistency to Dispatcher Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Keep the localized-text caches transient so the provider stays serializable Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Add missing Javadoc and cache-rebuild coverage for i18n provider Add the one-line Javadoc that sibling fields/setters carry to validateRequestLocale and its @Inject setter in Dispatcher, and add two tests covering StrutsLocalizedTextProvider's serialize/deserialize cache rebuild and cacheType selection behaviour. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Pin explicit serialVersionUID on the localized-text providers Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Suppress false-positive Sonar S3077 on the thread-safe i18n caches Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Drop the unused throws Exception from the new Dispatcher locale tests Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
81264063e6 |
WW-5659 Resolve lazy interceptor params per invocation (#1816)
* WW-5659 docs: design for request-scoped lazy interceptor params
WithLazyParams#injectParams resolves ${...} params onto the interceptor
singleton, so concurrent requests can read one another's resolved values.
For ActionFileUploadInterceptor that means allowedTypes, allowedExtensions,
maximumSize and disabled can cross between requests.
Design fixes the contract rather than the one implementer: resolved params
go into a per-invocation holder the interceptor supplies and receives back,
leaving the singleton immutable after init(). Adds InterceptorParams as the
general contract with DisableParams as opt-in support for the disabled param,
and makes unresolvable expressions fail closed instead of silently disabling
validation.
Reported via GitHub PR #1815; that approach (ThreadLocal on the interceptor)
is not adopted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 docs: implementation plan for request-scoped lazy params
Six tasks, each independently testable and compiling: new InterceptorParams
and DisableParams types, LazyParamInjector.resolveInto alongside the old
path, a pure refactor onto a single UploadPolicy value object, the contract
switch plus DefaultActionInvocation wiring, fail-closed handling, and test
migration.
Records one deviation from the spec: the unresolved-param rule is applied
unconditionally rather than by introspecting the seeded value, which is not
implementable deterministically for the Long-typed maximumSize. Task 6
updates the spec to match.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 docs: clarify test base class constraint in the plan
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 feat(core): add InterceptorParams contract and DisableParams holder
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 feat(core): resolve lazy params into a holder instead of the interceptor
* WW-5659 docs(core): correct isUnresolved javadoc and pin empty-value fail-closed behavior
isUnresolved cannot distinguish a failed ${...} resolution from an expression that
legitimately evaluates to an empty string; the parser gives no other signal. The
previous javadoc wrongly claimed the raw template let it tell the two apart. Fix
the javadoc to state the actual, intentional rule (fail-closed: treat both as
unusable), and add a test pinning that a legitimately-empty expression is treated
as unresolved rather than written.
* WW-5659 refactor(core): hold upload policy in one value object
Introduce UploadPolicy (extends DisableParams) to consolidate the three
loose maximumSize/allowedTypes/allowedExtensions fields on
AbstractFileUploadInterceptor into a single config-time value object.
acceptFile now takes the effective policy as an explicit parameter
instead of reading interceptor-level state directly.
Pure refactor, no behaviour change: the existing setters still mutate
the shared singleton via configuredPolicy, and ActionFileUploadInterceptor
copies it once per invocation via copyConfiguredPolicy() before calling
acceptFile. This groundwork lets a later change route lazily-resolved
per-request params into the copy instead of the singleton.
* WW-5659 fix(core): resolve lazy interceptor params per invocation
Co-Authored-By: deprrous <sukhbatsuugii2004@gmail.com>
* WW-5659 test(core): cover both lazy-params skip branches and per-invocation disabled
The two skip branches in DefaultActionInvocation#invokeWithLazyParams had no
coverage: deleting either left the whole suite green. The only tests reaching
that method used LazyFoo/LazyFooWithStackParams, which declare no disabled
param, and MockLazyParams did not extend DisableParams, so the holder branch
was unreachable and shouldIntercept was always true.
Make MockLazyParams extend DisableParams and add two action configs that
isolate one branch each:
- LazyFooLazilyDisabled passes disabled as an interceptor-ref param, so it
reaches InterceptorMapping#getParams(), resolves onto the holder, and
exercises the DisableParams branch.
- LazyFooStaticallyDisabled sets disabled on the interceptor definition
instead. InterceptorBuilder only puts interceptor-ref params into the
mapping, so the holder never sees it and it can only be honoured through
ConditionalInterceptor#shouldIntercept.
Verified by deleting each branch in turn: each deletion fails exactly the one
test that targets it, and no other.
Also make testDisabledIsResolvedPerInvocation earn its name. It previously
asserted only that newLazyParams() returns a fresh object, never resolving
anything, and built a MyDynamicFileUploadAction it never used. It now routes
two actions through a real LazyParamInjector#resolveInto of a
disabled=${uploadDisabled} param and pins that one invocation's resolved flag
survives the other's, and that neither reaches the interceptor singleton.
* WW-5659 fix(core): reject uploads when the policy cannot be resolved
* WW-5659 test(core): exercise real lazy param resolution in dynamic upload tests
* WW-5659 fix(core): mark params unusable when a lazy value cannot be applied
resolveInto had two failure branches behaving oppositely. An unresolvable
${...} skipped the write and notified the holder, so a fail-closed holder such
as UploadPolicy could reject the upload. A value the holder's setter could not
accept — a non-numeric String for the Long maximumSize, say — also skipped the
write but notified nothing, leaving the policy reporting isUnresolved() == false
and maximumSize null, which acceptFile reads as "no size limit". The cap was
silently off and the file accepted: fail-open, through a branch the design never
enumerated.
Notify the holder from the catch block too, and record in the spec the general
rule that every path skipping a write must notify, so a future failure mode gets
checked against it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 chore(core): harden the policy sets and tidy the lazy params dispatch
UploadPolicy handed out the mutable HashSet built by commaDelimitedStringToSet,
which the copy constructor shares by reference with the configured policy, so a
subclass overriding the protected acceptFile could have rewritten process-wide
config from a request thread. The sets are unmodifiable now.
Also document that unresolved() records `disabled` like any other param, so an
unresolvable disabled expression leaves the interceptor enabled and rejects
every upload; and in DefaultActionInvocation use normal imports for the params
types, make the interceptor local final, word the three skip logs consistently
and identify the interceptor by its mapping name, and note that the name-based
param merge is inherited behaviour whose duplicate-ref handling is questionable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 fix(core): allowlist the lazy params holder for OGNL member access
Moving lazy param resolution off the interceptor and onto a per-invocation
InterceptorParams holder changed the OGNL target of the write. The interceptor
is allowlisted at configuration time by XmlDocConfigurationProvider, because it
is named in the configuration; the holder is named nowhere, so with the shipped
default struts.allowlist.enable=true SecurityMemberAccess refused every setter,
resolveInto's fail-closed handling marked every param unresolved, and
ActionFileUploadInterceptor rejected every upload.
Register the holder's own class hierarchy through ProviderAllowlist when the
interceptor is built, keyed by the holder class so repeated builds collapse onto
one entry. Only the holder's class, superclasses and interfaces are registered -
the setter may be declared on any of them and SecurityMemberAccess checks both
the target and the declaring class. Object is filtered out: it says nothing
about the holder and is excluded by default anyway. No package is allowlisted.
Also run Interceptor#init() before the holder is obtained, so newLazyParams()
sees a fully initialised interceptor.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 test(core): prove lazy params resolve with the OGNL allowlist enabled
Every other core test runs with struts.allowlist.enable=false - StrutsTestCaseHelper
turns it off by default and XWorkTestCaseHelper never loads default.properties -
so no core test could see the holder being blocked by SecurityMemberAccess. Only
the showcase DynamicFileUploadTest integration test exercised the production
setting, which is why the regression reached CI.
This test boots the dispatcher with the allowlist enforced and asserts both that
the holder hierarchy is registered at configuration time and that a ${...} param
actually lands on the policy rather than being reported unresolved. Reverting the
registration in DefaultInterceptorFactory fails it with the same
"Declaring class [UploadPolicy] ... is not allowlisted" warning seen in the
showcase failure.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 fix(core): stop an unresolvable disabled param from voiding the upload policy
UploadPolicy#unresolved recorded every param name, disabled included, so an
unresolvable <param name="disabled">${...}</param> marked the whole policy
unusable and rejected every upload of the invocation. That is not a safe
default: disabled is not a validation dimension. Its unresolved value is simply
false, which leaves the interceptor running and the rest of the policy intact,
so it cannot relax validation - recording it only invents a second failure mode.
Exclude it from the tracking that gates isUnresolved(), via a new
DisableParams#DISABLED_PARAM constant, and replace the javadoc that defended the
old behaviour. A param that is a validation dimension still voids the policy,
including when it fails alongside disabled.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 docs(core): state what the lazy param injector actually did
The two WARN messages in resolveInto claimed consequences the injector does
not control. The ReflectionException branch said the params were 'marked
unusable', but InterceptorParams.unresolved is a defaulted no-op, so only a
holder that overrides it - UploadPolicy does - degrades at all. The
unresolved-expression branch said the configured value was kept, which reads
as a sensible fallback when it is normally the unevaluated ${...} literal
applied at build time.
Both now report only the injector's own actions: the value was not written
and the holder was notified. The nuance about what the holder retains moves
to the javadoc, where there is room to state it accurately.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 feat(core): reject unknown lazy interceptor params at configuration time
A param name that no property on the params holder can accept was only
noticed per request: resolveInto caught the ReflectionException, warned, and
notified the holder - which for UploadPolicy means rejecting every upload of
every request behind a WARN. The names are fully known when the configuration
is parsed, so DefaultInterceptorFactory now fails with a ConfigurationException
naming the interceptor, the param and the holder type.
Only the interceptor-ref params are checked. InterceptorBuilder passes that
same map on to the InterceptorMapping, and DefaultActionInvocation.mergedParams
feeds it to resolveInto, so it is exactly the set that reaches the holder.
Params on the <interceptor> definition are applied to the interceptor instance
and never reach the mapping; checking them too would reject working config,
<param name="disabled"> on a definition being the obvious case.
The runtime handling stays as defence in depth. ConfigurationException is now
rethrown rather than swallowed by the generic catch, so the operator reads the
param name instead of "Caught Exception while registering Interceptor class".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 refactor(core): drop the deprecated single-arg executeConditional
The overload lost its last caller when the mapping name became available at
the call site, so an existing subclass override would have compiled and then
never run again - silently dead code, worse than a compile error. This branch
already changes the protected acceptFile signature, so keeping the one-arg
form for source compatibility was not consistent either.
Covered by a test asserting the surviving two-arg form is the extension point
and receives the mapping name.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 fix(core): keep configuration order when merging lazy interceptor params
mergedParams built a HashMap, so the order the configuration carries was
discarded and the order params were applied to the per-invocation holder was
whatever hashing produced. LinkedHashMap makes it deterministic and matches
the LinkedHashMap the InterceptorBuilder already assembles.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 fix(core): keep interceptor params serializable
Interceptor extends Serializable, so an interceptor holding its configured
params as a field must hold something serializable. The fields UploadPolicy
replaced were a Long and two HashSets, all serializable; the holder was not,
which silently broke serialization of every file upload interceptor.
Fix it on the contract rather than the field: InterceptorParams now extends
Serializable, so every holder inherits the requirement. Marking the field
transient would instead have dropped the configured policy on deserialization.
Also renames four test locals that shadowed the interceptor field and drops
a throws clause that could not be reached, both reported by SonarCloud.
* WW-5659 test(core): hoist the params map out of the assertThatThrownBy lambdas
Each lambda called both params(...) and buildInterceptor(...), so a throw from
the helper would have satisfied the assertion just as well as one from the code
under test. Building the map first leaves one throwing call per lambda.
Reported by SonarCloud (java:S5778).
* WW-5659 fix(core): stop seeding the interceptor with raw lazy expressions
A ${...} param is resolved per invocation into the params holder, so applying
its raw text to the interceptor at configuration time only seeded an
unevaluated literal - allowedTypes held "${uploadConfig.allowedMimeTypes}",
matching no content type - or failed conversion outright for a typed property
such as the Long maximumSize.
Withhold those params at build time; static params still apply and still seed
the holder, which is what a lazy param falling back has to fall back to. Also
makes InterceptorParams.unresolved's javadoc about the retained value honest.
Idea from @deprrous in GitHub PR #1815.
Co-Authored-By: deprrous <sukhbatsuugii2004@gmail.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: deprrous <sukhbatsuugii2004@gmail.com>
|
||
|
Lukasz Lenart
|
94a8fcb26c |
WW-3784 Order annotated wildcard actions most-specific-first (#1813)
* WW-3784 docs: design for specificity-ordered wildcard matching in annotated actions Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 docs: implementation plan for annotated wildcard specificity ordering Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 feat(convention): add action-name specificity comparator Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 fix(convention): add Apache License header to test file Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 feat(core): add PackageConfig.Builder.reorderActionConfigs Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 docs: add javadoc for PackageConfig.Builder.reorderActionConfigs Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 feat(convention): order annotated wildcard actions most-specific-first Sorts each convention-built package's action configs by pattern specificity so a specific pattern (some/usefull/*) is matched before a general one (some/*), regardless of class-scan order. Also makes convention action ordering deterministic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 docs: correct wildcard cross-segment claims and note comparator limitations The spec incorrectly stated that WildcardHelper's single `*` is greedy and crosses `/`, and that `some/*` shadows `some/usefull/*`. Verified against WildcardHelper.java and NamedVariablePatternMatcher.java: only `**` crosses `/`, so those two patterns are actually disjoint (different segment counts) and never compete for the same request. Correct the Problem narrative, ticket example, and matcher bullets to state this accurately, and document two known limitations of the specificity comparator (raw wildcard-token-count key can misrank `**` ahead of narrower multi-token patterns; parent-package actions bypass sorting). Also add a test asserting the natural-order alphabetical tiebreak key. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 test(convention): prove specificity ordering fixes wildcard shadowing end-to-end Adds an end-to-end routing test driving the production reorder (PackageConfig.Builder.reorderActionConfigs + ActionNameSpecificityComparator) through the real ActionConfigMatcher/WildcardHelper. some/** and some/usefull/* genuinely overlap for some/usefull/sleeping (** crosses '/'), so the test asserts the general pattern shadows the specific one when registered first, and that specificity ordering makes the specific action reachable again. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
6f802987f5 |
WW-3871 Derive ConversionRule prefixes for @TypeConversion keys (#1812)
* WW-3871 docs: add design spec for @TypeConversion key derivation Specifies deriving the ConversionRule prefix for @TypeConversion keys at class, method and field level via a single resolver, adds ElementType.FIELD as a target, and records the break/continue and empty-key fixes in the same code block. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * WW-3871 docs: note interaction with the 7.3.0 converter mapping cache Records that addConverterMapping runs inside the computeMappingIfAbsent builder introduced by WW-5539, which executes outside any lock, so the new field pass adds no deadlock risk but must stay side-effect free. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * WW-3871 docs: add implementation plan for @TypeConversion key derivation Seven TDD tasks covering ConversionRule#prefix(), the shared resolveKey helper, class- and field-level derivation, the break/continue and empty-key fixes, an end-to-end binding proof and the Javadoc updates. Refines the spec's resolveKey signature to take the two annotation attributes rather than the annotation instance, so it can be unit tested directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * WW-3871 feat(core): add ConversionRule#prefix() owning the rule-to-prefix table * WW-3871 refactor(core): split addConverterMapping into per-source passes * WW-3871 feat(core): derive conversion mapping keys through a single resolver * WW-3871 fix(core): derive class level conversion keys and stop dropping later entries * WW-3871 feat(core): support @TypeConversion on fields * WW-3871 test(core): assert bare conversion keys bind through the action lifecycle Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * WW-3871 docs(core): document conversion key derivation and field level support * WW-3871 docs(core): add deprecated Collection_ prefix to parameter table * WW-3871 fix(core): widen resolveKey idempotence guard against any rule prefix resolveKey only recognized a key as already-prefixed if it started with its own declared rule's prefix. COLLECTION and ELEMENT are interchangeable throughout the conversion pipeline (DefaultConversionAnnotationProcessor handles them in the same branch, DefaultObjectTypeDeterminer.getElementClass reads Element_ then falls back to the deprecated Collection_), so key="Element_users" with rule=COLLECTION silently doubled to Collection_Element_users instead of being left alone, losing the mapping. Match against every known rule's prefix instead. Also documents two related precedence subtleties surfaced during review: processFieldAnnotations' Javadoc now notes that an inherited method can claim a key before a subclass's own field annotation is considered, since getMethods() includes inherited methods and runs first; and the unresolvable-key WARN in processMethodAnnotations now names the method's declaring class rather than the class being scanned, since getMethods() can surface the same inherited method at every level of the hierarchy. Design spec section 2 updated to match the implementation. * WW-3871 docs(core): correct TypeConversion Javadoc property attribute and determiner package Two pre-existing errors in the block this ticket's commits already touch: the APPLICATION example used a non-existent "property" attribute where "key" is the working form (see ConversionTestAction.java:97), and the rule() Javadoc pointed at org.apache.struts2.util.DefaultObjectTypeDeterminer instead of the actual org.apache.struts2.conversion.impl package. * WW-3871 test(core): cover key-prefix crossover, empty class-level key, and KeyProperty_ end-to-end binding - testResolveKeyLeavesAnAlreadyPrefixedKeyAlone: add the COLLECTION/ELEMENT crossover cases that demonstrate the resolveKey guard fix (fail before, pass after). - New EmptyKeyConversionAction fixture plus testClassLevelEmptyKeyRegistersNoMapping: a class-level @TypeConversion with no key must be skipped, not registered under "". This was the one behavioural bullet in the spec's test plan with no coverage. - MyBeanActionTest.testBareConversionKeysBindTheSameWayAsPrefixedOnes: add an assertion that the bare KeyProperty_ derivation actually binds the list index onto the created bean's id property end to end, not just that a converter mapping exists. * WW-3871 fix(core): skip APPLICATION-scoped @TypeConversion with no explicit key Method- and field-level @TypeConversion(type = APPLICATION) with no key previously derived a member name (e.g. a setter's property name) and registered it in the global default converter map via addDefaultMapping. That map is only ever read by class name (lookup(String, boolean) and lookup(Class)), so the entry was permanently unreachable. Skip it before deriving a name, logging a WARN naming the declaring class and member; the class-level pass already handled this correctly via resolveKey returning null. Adds a fixture and tests proving no default mapping is registered under the derived member name in either pass. * WW-3871 docs(core): fix broken TypeConversion Javadoc example and align spec TypeConversion's example class declared `users` twice (once unannotated, once again at its annotated field), so the sample no longer compiled as written; drop the earlier, redundant declaration. The same example's setConvertInt showed @TypeConversion(type = APPLICATION) with no key - exactly the case the previous commit's XWorkConverter fix now skips. Drop the type attribute so it reads as a class-scoped conversion, matching the corrected ConversionTestAction fixture. The correct APPLICATION example further down (execute(), key = "java.util.Date") is untouched. Also records the APPLICATION no-key skip rule in the design spec's carve-out paragraph so spec and code agree. * WW-3871 fix(core): dedupe method-pass WARN logging for inherited @TypeConversion processMethodAnnotations iterates clazz.getMethods(), which includes inherited public methods, and buildConverterMapping calls it once per class in the hierarchy. A single misconfigured @TypeConversion on a base class method was therefore logging its WARN once per subclass level. Gate both WARN call sites on method.getDeclaringClass() == clazz so each fires exactly once, at the level that owns the method; the derivation/registration logic keeps running on every visit unchanged. Adds a small permanent test proving the gate is logging-only: an inherited annotated setter still resolves and registers through a subclass that overrides nothing. * WW-3871 docs(core): clarify field-name key default and dedicated-annotation precedence Two gaps in the @TypeConversion Javadoc, both newly relevant now that the annotation targets fields: - The key() default on a field is the field name, not the JavaBean property name (processFieldAnnotations uses field.getName()). A field like _users backing property users would otherwise derive CreateIfNull__users, a key DefaultObjectTypeDeterminer never looks up. - org.apache.struts2.util's dedicated field annotations (@Key, @Element, @KeyProperty, @CreateIfNull) are consulted by DefaultObjectTypeDeterminer before it falls back to the converter mapping @TypeConversion populates, so a dedicated annotation silently wins over an equivalent @TypeConversion on the same property. Verified against getAnnotation/getElementClass/ getKeyProperty in DefaultObjectTypeDeterminer before documenting it. * WW-3871 docs(core): note COLLECTION derives the deprecated Collection_ prefix ConversionRule.COLLECTION.prefix() intentionally returns Collection_, the spelling DefaultObjectTypeDeterminer treats as deprecated and logs an INFO about on every fallback hit, kept for compatibility with existing annotations. Document that the derivation is deliberate and point readers at ELEMENT as the current form. * WW-3871 refactor(core): extract shared annotation-registration pipeline processMethodAnnotations and processFieldAnnotations were the same five-step pipeline (skip non-@TypeConversion, skip APPLICATION-scoped without a key, derive the name, resolve the key, register unless already mapped) written twice, driving SonarCloud S3776 cognitive complexity to 26 and 21 respectively and triggering three S135 multiple-break/continue findings. Extract steps 2-5 into a private registerAnnotatedMember(mapping, tc, Member, fallbackName, logSkips) helper that both passes delegate to. Each pass is now just its loop plus one instanceof check. The method pass keeps its per-declaring-class log gate (getMethods() revisits inherited methods once per hierarchy level); the field pass always logs, since getDeclaredFields() is visited once per class. The two WARN wordings, which differed only in a trailing clause, are merged into one message accurate for both a method and a field. No change to the registered mapping, pass order, or precedence for any class - verified via the existing XWorkConverterTest, AnnotationXWorkConverterTest, MyBeanActionTest, and ConversionRuleTest suites (92 tests, same count and same triggering warnings before and after) plus the full core module suite (3043 tests). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * WW-3871 docs(core): fix inaccurate and self-contradicting TypeConversion key Javadoc @Key, @Element, @KeyProperty and @CreateIfNull are @Target({FIELD, METHOD}), not field-only, and the same paragraph already says they are read from the field, setter and getter - drop "field" from "dedicated field annotations". Fold the field-vs-property-name correction into key()'s opening sentence instead of stating "defaults to the property name" and rebutting it three lines later, and align the parameters table row for key with the same rule. * WW-3871 docs(core): clarify XWorkConverter annotation-registration logging Give the success DEBUG the same [declaringClass#member] shape the three skip messages already use, instead of logging the bare member name that identifies neither the class nor whether it was a method or a field. Reword the "already mapped" DEBUG so it covers its commonest trigger - the same annotation seen one hierarchy level down, not just a genuinely higher-precedence source. Note in the logSkips comment that buildConverterMapping only visits each class' direct interfaces, so a misconfigured annotation declared on a super-interface method never gets logged at all, even though registration is unaffected. Also drop a stray extra blank line. No behavioural change: registration/derivation logic is untouched. * WW-3871 test(core): make inherited-method-annotation test diagnostic testInheritedMethodAnnotationStillRegistersThroughASubclass previously asserted nothing the logSkips gate could break: the hierarchy walk always reaches InheritedMethodConversionAction itself, where declaringClass == clazz, so the key registers there regardless of whether registration is (wrongly) gated alongside logging. The test passed identically with logSkips hardcoded true or false. Give InheritedMethodConversionSubAction a contesting field annotation for the same property the inherited setter claims. The inherited method annotation registers at the subclass level - before the subclass's own field pass runs - so its value must keep winning; that is the invariant documented on processFieldAnnotations, and it is exactly what gating registration would break, since the subclass field would start winning over the inherited method annotation instead. Verified: temporarily wrapping the registerAnnotatedMember call in processMethodAnnotations with `if (logSkips)` makes this test fail (expected:<true> but was:<false>); reverting it passes again. Mutation was not committed. Corrected both Javadocs, which overclaimed what the old assertion proved. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
d906f23448 |
WW-3530 Fix visitor-validator cache-key collision under wildcard actions (#1811)
* WW-3530 docs: add design spec for visitor-validator cache-key fix Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 docs: add implementation plan for visitor-validator cache-key fix Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 test(core): cover visitor-validator cache-key context handling under wildcard actions Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 fix(core): keep visitor-validator context in cache key under wildcard actions Apply the wildcard config-name substitution only when validating the action's own class. Visited objects carry a stable, explicit visitor context that must remain part of the cache key, otherwise two visitor validators on one field with different contexts collide and the second is silently dropped. Fixes https://issues.apache.org/jira/browse/WW-3530 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 docs: document <s:form> render-path caching limitation Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 docs: correct WW-2996 scope claim and note default-context visitor limitation Final-review finding: default-context visitor validators under wildcard actions key on the volatile resolved action name for the visited class, reintroducing bounded WW-2996-style cache growth (memory only; correct validators still load). Correct the 'WW-2996 untouched' wording to 'untouched for the action's own class', document the subpath as an accepted limitation folded into the follow-up ticket, and clarify that end-to-end visitor execution is covered by existing visitor suites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 chore(core): add DEBUG logging for validator cache-key branch decision Log the built key together with clazz, context, validatingActionClass, wildcard, and the action config name, so the wildcard-vs-visited-object branch taken in buildValidatorKey can be diagnosed at runtime. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 test(core): use assertNotEquals for cache-key inequality; fix comment grammar Address SonarCloud S5785 (assertFalse+equals -> assertNotEquals) and a Copilot grammar nit in the WW-4536 comment. DEBUG logging kept as-is per author decision. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
532ca7f864 |
WW-2934 Skip field validators when a field has a conversion error (opt-in) (#1810)
* WW-2934 docs: add design spec for skipping validators on conversion error Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-2934 docs: add implementation plan for skipping validators on conversion error Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-2934 feat(core): skip field validators on conversion error behind opt-in flag Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-2934 test(core): cover annotation manager + document conversion-error skip exemption Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-2934 test(core): assert custom conversion message survives and cover nested field skip Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
12015d0bf5 |
WW-5540 Add caching to AbstractLocalizedTextProvider (#1808)
* WW-5540 docs: add caching design spec for AbstractLocalizedTextProvider Design for caching the class/package hierarchy traversal result in findText, keyed on (classloader, class name, textKey, locale). Caches the raw resolved pattern (or a NOT_FOUND marker) only; translation and formatting stay per-call. Wires invalidation into the existing reloadBundles/clearBundle/clearMissingBundlesCache sites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 docs: add implementation plan and refine spec Add the 3-task TDD implementation plan and record the formatWithNullDetection fall-through decision in the spec. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 docs: deprecate+delegate findMessage/getMessage in plan Resolve pre-flight duplication/dead-code finding: old traversal helpers delegate to the raw twins and are marked @Deprecated instead of being duplicated. Add a direct characterization test for the findMessage delegator. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 refactor(core): split raw message resolution from formatting Add getRawMessage/formatMessage and a raw twin findMessageRaw. Re-express getMessage via formatMessage and make findMessage delegate to findMessageRaw + formatMessage; deprecate both as legacy extension points superseded by the raw-resolution path. The deprecated findMessage triggers the bundle reload on entry, preserving the reload side effect the old getMessage-per-probe walk provided. Groundwork for the traversal caches. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 docs: refine Task 1 plan (deprecate/delegate + reload-on-entry) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 perf(core): cache class-hierarchy text resolution Cache the class/interface/superclass traversal in findText keyed on (classloader, class name, key, locale), storing the raw pattern or a NOT_FOUND marker. Formatting stays per call and falls through to the next tier when a cached pattern formats to null. Invalidated on reloadBundles/clearBundle/clearMissingBundlesCache; reload is hoisted to the top of findText so caches are cleared before they are read. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 docs: draft follow-up ticket for null-control-flow cleanup Capture the deferred result-wrapper refactor (raised during WW-5540) as a ready-to-file Jira draft; keep WW-5540 focused on caching. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 perf(core): cache package-hierarchy text resolution Cache the *.package traversal in findText the same way as the class hierarchy, with the same keying, fall-through, and invalidation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 test(core): tighten localized-text cache tests Assert single cache entry in the per-call-format tests (proves the raw pattern is cached, not the formatted result), and mirror the package-cache clearBundle/clearMissingBundlesCache invalidation test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 docs: note devMode null-valueStack eager-reload edge Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 docs: link follow-up doc to filed ticket WW-5655 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 chore(core): add ASF license header to CacheFixture.properties Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 chore(core): add since/forRemoval to @Deprecated annotations Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 docs: drop follow-up draft superseded by WW-5655 The ticket is filed; the draft's content now lives in WW-5655 itself. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 fix(core): address fresh-eyes review findings - Document that the deprecated getMessage/findMessage are no longer invoked by findText, and name formatMessage as the override point - Fall back to the ActionContext-based reloadBundles() when findText is called without a value stack, so the RELOADED flag is tracked and the caches can warm on that path in reload/devMode - Narrow resolveClassHierarchyRaw/resolvePackageHierarchyRaw to package-private (the cache key omits indexedKey, which is safe only when derived from textKey as the internal call sites do) - Suppress java:S2129 on the NOT_FOUND identity sentinel Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 docs: strip stray NUL bytes from design spec Two literal NUL bytes in the sentinel example made git/GitHub treat the whole markdown file as binary and unreviewable in the PR UI; align the example with the shipped sentinel name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 test(core): cover ModelDriven tier, per-locale keys, indexed keys Close the review-noted coverage gaps: the ModelDriven tier resolves via the shared class-hierarchy cache (action miss + model hit), each locale gets its own cache entry backed by a new _de fixture bundle, and indexed keys (name[N] -> name[*]) resolve and cache per full textKey. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 fix(core): address Copilot review comments - Partition the caches by System.identityHashCode of the context classloader so a custom ClassLoader overriding hashCode() cannot collide or collapse the per-loader partitions - Derive the indexed key inside the resolvers (miss-only) instead of accepting it as a parameter, so the cache key trivially covers every input that influences the resolution result Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 fix(core): resolve SonarCloud quality-gate findings - Suppress S4973 on isNotFound: the identity comparison against the non-interned NOT_FOUND sentinel is the design, not a bug - Reduce findMessageRaw cognitive complexity (S3776) by extracting getRawMessageWithAlternate, reused by the package walk - Add missing @Override annotations and suppress the deliberate deprecated-delegator call in the test helper (S1161, S5738) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 fix(core): make findMessageRaw cycle guard effective The `checked` set tested `contains(clazz.getName())` but never added the class, so the diamond-interface cycle guard was a no-op (a latent issue inherited from the original findMessage). Add the class name after the contains-check so repeated interface branches aren't re-traversed. Behavior is unchanged (lookups are idempotent); this only avoids redundant recursion. The deprecated findMessage delegates here, so it's fixed too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 docs(core): reference WW-5658 in deprecated method javadoc Point the @deprecated javadoc of getMessage and findMessage at WW-5658, the ticket tracking their removal in the next major release. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
833220346c |
WW-5474 Count files only for maxFiles, add maxParameterCount (#1806)
* WW-5474 docs(multipart): design for files-only maxFiles + maxParameterCount Spec for correcting struts.multipart.maxFiles to count file parts only (consistently across the jakarta and jakarta-stream parsers) and adding struts.multipart.maxParameterCount to cap non-file form fields, restoring the DoS guard the old accidental total-part cap provided. Fail-closed on breach. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5474 docs(multipart): implementation plan for maxFiles/maxParameterCount Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5474 fix(multipart): count files only for maxFiles, add maxParameterCount (jakarta) The jakarta parser passed maxFiles to commons-fileupload2 setMaxFileCount, which counts every part (fields + files), so maxFiles wrongly limited total parameters. Enforce a files-only count and non-file field count in Struts, failing closed on breach; keep a total-parts commons backstop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5474 fix(multipart): honor -1 unlimited sentinel in total-parts backstop prepareServletFileUpload applied the total-parts backstop whenever both maxFiles and maxParameterCount were non-null, without checking for the -1 "unlimited" sentinel already honored by enforceMaxFiles/enforceMaxParameterCount. With maxFiles=-1 and maxParameterCount=256, maxParts computed to 255 and was passed to commons-fileupload2's setMaxFileCount (which counts ALL parts), wrongly rejecting large file-only uploads. Only apply the backstop when both limits are finite (non-null and >= 0). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5474 fix(multipart): apply files-only maxFiles + maxParameterCount to stream parser Replace the field-name-based exceedsMaxFiles with the shared files-only enforcement and add parameter-count enforcement, matching the jakarta parser and failing closed on breach. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5474 fix(multipart): track all parsed items to avoid temp-file leak on fail-closed breach servletFileUpload.parseRequest() fully materializes every part - spilling large ones to disk - before processUpload() iterates over the result. The loop only added each DiskFileItem to diskFileItems as it was reached, so when enforceMaxFiles/enforceMaxParameterCount threw mid-loop on a breach, every item positioned after the breaching one was never registered for cleanup. With no FileCleaningTracker on the factory, cleanUp() had no way to reclaim those temp files, leaking disk space on the hardening path. Materialize the parsed list once and register all items for cleanup before processing so cleanUp() reclaims every temp file regardless of where enforcement aborts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5474 fix(multipart): guard debug logging in enforce helpers (Sonar S2629) Wrap the LOG.debug calls in enforceMaxFiles/enforceMaxParameterCount with isDebugEnabled() so normalizeSpace() is not evaluated when debug is disabled, matching the exceedsMaxStringLength pattern in the same class. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5474 fix(multipart): address Copilot review - parser parity + overflow guard - JakartaMultiPartRequest: only count/enforce a file part toward maxFiles when it has a non-null field name, matching JakartaStreamMultiPartRequest's accept criteria (defensive: commons-fileupload2 already drops parts without a name attribute before parseRequest returns, so the two parsers stay consistent regardless). - AbstractMultiPartRequest: compute the total-parts backstop with Math.addExact and clamp to Long.MAX_VALUE on overflow, so extremely large configured limits cannot wrap negative and silently disable the commons backstop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5474 fix(multipart): log "processing a form field" only for form fields Move the debug log into the isFormField branch so file parts are not mislabelled; the file branch already logs "Processing a file". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
c3e9db1b21 |
WW-5413 Avoid writing small in-memory multipart uploads to disk (#1805)
* WW-5413 docs(core): design for in-memory multipart upload optimization Lazy-materializing UploadedFile plus a new getInputStream() accessor so small (in-memory) uploads no longer eagerly write a temp file, while getContent() keeps returning a File for backward compatibility. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5413 docs(core): implementation plan for in-memory upload optimization Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5413 feat(core): add UploadedFile.getInputStream() streaming accessor Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5413 feat(core): add lazily-materializing StrutsInMemoryUploadedFile Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5413 fix(core): make StrutsInMemoryUploadedFile serializable and thread-safe * WW-5413 refactor(core): drop eager temp-file write for in-memory uploads * WW-5413 test(core): cover deferred-write behavior for in-memory uploads Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5413 perf(core): avoid materializing in-memory uploads during interceptor validation * WW-5413 chore(core): clean up partial materialization and cover isMissing() * WW-5413 docs(core): sync design/plan with interceptor fix and deviations Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5413 chore(core): deprecate now-unused STRUTS_MESSAGES_INVALID_CONTENT_TYPE_KEY Mark the orphaned constant @Deprecated(forRemoval = true) instead of leaving it silently unused. The message key it referenced was only emitted from an unreachable block in acceptFile() that was removed with the in-memory upload optimization. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5413 test(core): cover materialization failure and getInputStream default branches Address review follow-ups on PR #1805: - document that processFileField's retained 'throws IOException' is intentional (subclass source compatibility), not an oversight - add a negative test: getContent() on an unwritable save dir throws StrutsException, stays unmaterialized, and leaves no partial file behind - cover the UploadedFile.getInputStream() default File branch and the no-content IOException branch Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5413 fix(core): address SonarCloud and Copilot review findings - materialize() now writes with StandardOpenOption.CREATE_NEW and fails closed if the target already exists, so a pre-planted file/symlink is never overwritten or followed (Copilot security note) + regression test - defensively copy the content byte array on construction and reject null content, so the instance owns its bytes and cannot observe caller mutation (Copilot / review) - delete() uses Files.deleteIfExists and logs the real cause on failure instead of a silent File.delete() boolean (Sonar MAJOR) - reorder field modifiers to JLS order 'transient volatile' (Sonar) - tests: assertThat(dir).isEmptyDirectory() instead of listFiles().isEmpty() (Sonar); drop unused DiskFileItem import (Sonar) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
8a5323fbdf |
WW-5539 Concurrency performance enhancements (#1799)
* WW-5539 docs: add concurrency performance enhancements design Design for removing coarse locks from XWorkConverter, DefaultActionValidatorManager and StrutsTypeConverterHolder in favour of concurrent collections. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 docs: add concurrency performance implementation plan Five tasks derived from the approved design: make StrutsTypeConverterHolder concurrent, add the computeMappingIfAbsent SPI method, remove the locks from XWorkConverter and DefaultActionValidatorManager, then benchmark and raise the PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 docs: make conditionalReload store its rebuilt mapping buildConverterMapping no longer stores its result, so reload mode would have rebuilt from disk on every request without ever caching. * WW-5539 Make StrutsTypeConverterHolder collections concurrent The holder is a container singleton whose HashMaps were read without any lock by XWorkConverter.lookup() while being written elsewhere, risking lost updates and torn reads during resize. Null TypeConverters are now ignored with a warning rather than stored, since ConcurrentHashMap forbids null values and a null converter left the holder in an inconsistent state. * WW-5539 Rename test to match what it actually covers The method exercised only the unknown-mapping cache, not noMapping. * WW-5539 Add TypeConverterHolder#computeMappingIfAbsent Adds an atomic build-once-and-cache operation so callers no longer need check-then-act around the class mapping cache, and deprecates the three primitives it subsumes: getMapping, addMapping and containsNoMapping. The method is a default method delegating to those primitives, so third-party TypeConverterHolder implementations keep working unchanged. * WW-5539 Deduplicate the no-mapping path in computeMappingIfAbsent ConcurrentHashMap.computeIfAbsent stores nothing when the mapping function returns null, so every concurrent caller re-ran the builder for a class with no conversion mapping - the common case for an ordinary action, and the exact thundering herd this method exists to prevent. Negative results now store a sentinel in the same map, so the builder runs once per class either way. getMapping and containsNoMapping translate the sentinel, preserving their existing contracts. * WW-5539 docs: sync plan with negative-cache sentinel fix * WW-5539 Pin down addNoMapping's override semantics Storing the no-mapping sentinel deliberately replaces any mapping cached for the class, matching the pre-7.3.0 effective behaviour where such a class was short-circuited before its cached mapping was ever read. putIfAbsent would instead serve a stale mapping after a failed build. Also asserts the sentinel translation in getMapping directly, and stops the interface javadoc promising a specific empty-map instance that implementations are not required to return. * WW-5539 Document that addNoMapping may replace a cached mapping The behaviour was documented only on the Struts implementation, but addNoMapping stays a non-deprecated SPI primitive that third parties both call and implement, so the contract belongs on the interface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 Remove coarse locks from XWorkConverter getConverter() synchronized on the Class object being converted, which is a globally visible monitor any other library may contend on, and which serialised every conversion for a given action class including cache hits. It now delegates to TypeConverterHolder#computeMappingIfAbsent. registerConverter and registerConverterNotFound drop their synchronized modifier; they are single delegations to a concurrent map, and the lock never covered the readers in lookup() in any case. buildConverterMapping no longer stores its result - storage is owned by computeMappingIfAbsent. * WW-5539 Remove global lock from DefaultActionValidatorManager getValidators() was synchronized on the singleton manager, so every validated request in the application serialised on it - and the lock covered the per-request Validator construction loop, which operates on per-request objects and never needed mutual exclusion. Both caches become ConcurrentHashMap and cached config lists are wrapped unmodifiable, since several threads now iterate them concurrently. * WW-5539 Make the validator concurrency test race a cold cache The test computed its expected count with a getValidators call before starting the threads, which warmed the cache and left all 16 workers on the fast path - never exercising first-build contention, the race the test is named for. Also drops an unused import and awaits executor termination. * WW-5539 Address final review findings Restores the protected unknownMappings field verbatim as a deprecated, unused vestige: retyping it changed the field descriptor, so a subclass compiled against 7.2.0 would have hit NoSuchFieldError on upgrade without recompiling. Real storage moves to a private concurrent set. Also stops conditionalReload running for negative-cached classes, which had been costing a failed classloader resource scan per property per request in devMode, and restores the unknown-mapping clearing that the null-converter guard was skipping. * WW-5539 Fix concurrency regressions from coarse-lock removal Four correctness fixes surfaced in PR review of the concurrent-collections refactor: - StrutsTypeConverterHolder.addDefaultMapping: restore put-before-remove ordering. The inverted order let a concurrent XWorkConverter.lookup observe (unknown=false, default=false), sending it into lookupSuper() and letting it overwrite the more specific converter being registered. - StrutsTypeConverterHolder.computeMappingIfAbsent: stop building inside a ConcurrentHashMap bin lock. The builder reaches ObjectFactory.buildConverter, which can autowire arbitrary user TypeConverters; running that under a CHM bin lock risked a recursive-update exception or self-deadlock. Callers now only get the guarantee that they converge on the same cached instance, not that the builder runs exactly once - documented on the interface and reflected in the concurrency tests. - DefaultValidatorFactory.validators: switch to ConcurrentHashMap now that DefaultActionValidatorManager.getValidators is no longer synchronized, so runtime registerValidator() calls no longer race unsynchronized reads of a plain HashMap. - XWorkConverter.conditionalReload: route empty devMode rebuilds through addNoMapping instead of addMapping, so an empty reload result is stored as the NO_MAPPING sentinel rather than a plain empty map that would silently disable further reloads for the class. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 Add tests closing coverage gaps from coarse-lock removal SonarCloud's quality gate failed at 59.8% coverage on new code (need >=80%). Adds tests for the specific lines JaCoCo identified as uncovered, without touching production code: - TypeConverterHolder.computeMappingIfAbsent's default method body (the SPI compatibility fallback for third-party holders that predate 7.3.0 and don't override it) - new TypeConverterHolderTest against a minimal non-overriding implementation. - StrutsTypeConverterHolder.getMapping/containsNoMapping's remaining non-sentinel branch. - XWorkConverter.conditionalReload's reloadingConfigs==true path (both the addMapping and addNoMapping outcomes), buildConverterMappingUnchecked's checked-to-IllegalStateException wrapping, and getConverter's catch(Throwable) negative-caching. - DefaultActionValidatorManager's else-if(reloadingConfigs) cache rebuild, loadFile's checkFile&&fileNeedsReloading re-parse, and buildValidatorConfigs' already-checked short-circuit. TypeConverterHolder.java and StrutsTypeConverterHolder.java are now at 0 missed lines/branches. XWorkConverter.java and DefaultActionValidatorManager.java have all requested target lines covered; remaining misses are pre-existing, unrelated gaps left alone per scope. Full core suite: 3026 tests (3015 + 11 new), 0 failures/errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 Fix SonarCloud deprecation and test-hygiene issues - Add since/forRemoval attributes to the 7 @Deprecated elements on TypeConverterHolder.getMapping/addMapping/containsNoMapping and StrutsTypeConverterHolder's overrides plus the unknownMappings field (java:S6355). - Add the missing @deprecated Javadoc tag to the three StrutsTypeConverterHolder overrides, pointing at computeMappingIfAbsent as the replacement (java:S1123). - Remove the unused throws Exception from testGetConverterBuildsMappingExactlyOncePerClass (java:S1130). - Document why StubFileManager.setReloadingConfigs/monitorFile are intentionally empty no-ops (java:S1186). - Rename a local variable that shadowed the converter field in testConditionalReloadRebuildsEmptyMappingAndStoresItViaAddNoMapping (java:S1117). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 Suppress removal warnings for the deprecated holder primitives javac treats [removal] as a category separate from [deprecation], so marking the three primitives forRemoval left four warnings behind: the deliberate addMapping call in conditionalReload, and the three overrides that must exist for as long as the interface declares them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 Address Copilot review comments Fixes getMapping's @return (it returns a Map, not a TypeConverter) and drops the "atomically" wording from its @deprecated tag, which no longer matches computeMappingIfAbsent's contract now that the builder may run more than once under concurrent first access. Syncs the design and plan docs with the shipped approach: the unknownMappings field is kept for binary compatibility rather than retyped, and the override uses get/build/putIfAbsent rather than computeIfAbsent. * WW-5539 docs: correct the classloader out-of-scope note The conversion caches are container-scoped singletons with no external references, so their Class keys do not independently pin the webapp classloader - that is governed by whatever retains the container (WW-5537). Reframed as optional defense-in-depth cache clearing, folded into WW-5537 Task 5b, rather than a standalone leak fix. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
464817e0b0 |
WW-5653 Upgrade Bootstrap to 5.3.x in sample apps (#1793)
* WW-5653 docs: add Bootstrap 5.3.x sample-app migration design Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 docs: add Bootstrap 5 migration implementation plan Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 build: add Bootstrap 5, Bootstrap Icons, showcase jQuery webjars Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 feat(showcase): serve Bootstrap 5 and jQuery via webjars Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 fix(showcase): serve html5 demo Bootstrap CSS via webjar Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 feat(showcase): migrate navbar and top-level pages to Bootstrap 5 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 fix(showcase): migrate leftover Bootstrap 2 icon and fixed-navbar classes Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 feat(showcase): migrate tag-demo pages to Bootstrap 5 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 feat(showcase): migrate validation-demo pages to Bootstrap 5 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 feat(showcase): migrate fileupload and conversion pages to Bootstrap 5 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 feat(showcase): migrate wait, token and empmanager pages to Bootstrap 5 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 fix(showcase): remove BS3 carets and well class Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 fix(showcase): use ms-auto for right nav and add nav-link to Home Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 feat(rest-showcase): serve Bootstrap 5 CSS via webjars Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 feat(rest-showcase): migrate JSP markup to Bootstrap 5 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 fix(rest-showcase): migrate legacy BS2/BS3 grid classes to Bootstrap 5 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 fix(showcase): drop obsolete css/js excludePattern so webjar JS is served The old struts.action.excludePattern whitelisted the vendored /styles/*.css and /js/*.js layout for container serving. After moving Bootstrap/jQuery to webjars under /static/webjars/**, the '.*/js/.*\.js' entry matched the webjar JS path (e.g. bootstrap.bundle.min.js) and excluded it from Struts' static handler, so it fell through to the container and 404'd. Remaining webapp assets (prettify.js, main.css) are served via default-servlet fall-through. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 fix(samples): migrate remaining BS2/BS3 classes (tables, buttons, progress, forms, navbar) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 chore(showcase): comment out verbose debug loggers Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 fix(showcase): migrate Bootstrap 5 markup in ftl/vm/html templates Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 test(showcase): disable JS in FreeMarkerManagerTest for Bootstrap 5 HtmlUnit's JS engine cannot parse Bootstrap 5's ES6 (bootstrap.bundle.min.js uses 'class'), and the decorator now serves it, so the default WebClient threw on script error. The test only asserts server-rendered FreeMarker output, so JavaScript is disabled (matching Html5TagExampleTest). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 fix(showcase): replace dead Bootstrap 2 form and alert classes Migrate new-person.ftl form off BS2 control-group/controls/form-actions to Bootstrap 5 (mb-3, form-label, form-control), and replace the dead alert-error class with alert-danger across the showcase pages. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 test(showcase): make integration tests tolerate Bootstrap 5 under HtmlUnit HtmlUnit 5.2.0 cannot parse Bootstrap 5's minified ES6 (bootstrap.bundle.min.js uses 'class'), which broke the showcase HtmlUnit integration tests once the decorator started serving the bundle. - Add ParameterUtils.createWebClient() which disables throwExceptionOnScriptError, and route all integration tests through it (they assert server-rendered output, not Bootstrap's client-side behaviour). - Load bootstrap.bundle.min.js with 'defer' so a page's own inline scripts (e.g. the async chat demo) still execute before HtmlUnit hits the bundle's parse error; defer is also the recommended real-browser loading strategy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 test(showcase): poll for async chat result instead of fixed sleep AsyncTest relied on a fixed Thread.sleep(4000) for the server-push chat round-trip, which is not enough on slower/newer JVMs (reproduced failing on JDK 25). Poll the result element for up to ~30s via waitForBackgroundJavaScript instead, making the test robust across JVMs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 test(showcase): drive AsyncTest via HTTP instead of HtmlUnit The browser-driven chat test was flaky on JDK 25 in CI: HtmlUnit's handling of the async server-push long-poll timed out (message never rendered), even with polling. The test's purpose is to validate the Servlet 3 async endpoints, which needs no browser or JavaScript. Rewrite it to POST /async/sendMessage and read /async/receiveNewMessages directly over HTTP and assert the JSON, making it deterministic and independent of Bootstrap/HtmlUnit JS parsing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 fix(showcase): drop progressbar role from wait progress bar Resolves a SonarCloud accessibility finding (S6819) introduced by the Bootstrap 5 migration. The BS5 progress component is styled on .progress/.progress-bar divs; the role/aria attributes were newly added (the BS3 original had none), so removing them clears the finding while keeping the Bootstrap 5 styling. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 fix(showcase): correct BS5 alert wrapper and drop stray table tag Address Copilot review on PR #1793: - decorators/main.jsp: the dismissible wrapper was a second .alert with no variant while the variant class (alert-danger, etc.) stayed on the inner <ul>, producing a nested, uncoloured alert box. Move the alert* classes onto the wrapper and strip them from the <ul> so the wrapper is the single alert container. - orders-edit.jsp: remove the stray, unclosed <table> start tag before </s:form> (pre-existing invalid markup carried over during the migration). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 fix(showcase): remove BS3 navbar-header wrapper and orphan dropdown-submenu Clean up two leftover Bootstrap 3 artifacts in the showcase decorator navbar: - Drop the `navbar-header` wrapper (no BS5 CSS behind it) and make the brand and toggler direct children of the `.container-fluid` flex container (justify-content: space-between), with the brand first per BS5 convention. - Remove the empty, unclosed `<li class="dropdown-submenu">` orphan before the first item in the Examples menu; BS5 has no dropdown-submenu feature. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 fix(showcase): replace soft-deprecated navbar-light with data-bs-theme navbar-light is soft-deprecated in Bootstrap 5.3. Switch the showcase navbar to the current data-bs-theme="light" idiom; bg-light is retained for the background. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 fix(showcase): normalize page-header replacement to border-bottom utilities A handful of showcase pages replaced the BS3 page-header with a bare <div> while the rest used <div class="border-bottom pb-2 mb-3">. Normalize those 35 header wrappers to the same border-bottom pb-2 mb-3 utilities so all showcase page headers render consistently. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5653 fix(rest-showcase): add mb-3 to page-header replacement Align rest-showcase order page headers with the showcase standard by using border-bottom pb-2 mb-3 (was border-bottom pb-2), so header spacing is consistent across both sample apps. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
11c10ee8f9 |
WW-5620 Standardize logging on Log4j2 (#1794)
* WW-5620 docs: add Log4j2 logging standardization design spec Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5620 docs: add Log4j2 logging standardization implementation plan Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5620 Migrate FinalizableReferenceQueue to Log4j2 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5620 Migrate AbstractDefaultToStringRenderable to Log4j2 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5620 Remove unused injectable j.u.l.Logger DI factory from ContainerBuilder Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5620 Remove dead first-party SLF4J dependency declarations Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
525c7dae37 |
WW-4858 Honor parameter filtering during JSON population (#1773)
* WW-4858 docs(json): design for honoring parameter filtering during JSON population Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-4858 docs(json): implementation plan for JSON parameter filtering Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-4858 feat(json): enforce excluded/accepted name patterns on JSON population Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-4858 feat(json): enforce param-name max length on JSON population Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-4858 feat(json): honor ParameterNameAware and ParameterValueAware on JSON population Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-4858 feat(json): add opt-in excluded/accepted value patterns on JSON population Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-4858 feat(json): opt-in applying excludeProperties/includeProperties to JSON input Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-4858 test(json): cover nested and list-element paths; clarify filter comments Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
5c130f9411 |
WW-5641 Restore struts.json.writer / struts.json.reader override in JSON plugin (#1766)
* WW-5641 docs: design spec for JSON writer/reader override regression Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5641 docs: implementation plan for JSON writer/reader override fix Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5641 fix: run JSON bean-selection from struts-deferred.xml The JSON plugin declared <bean-selection> in struts-plugin.xml, which runs at plugin-parse time, before the application struts.xml is folded in. That froze the JSONWriter/JSONReader default binding to StrutsJSONWriter/Reader, so struts.json.writer / struts.json.reader overrides were ignored. Move the element to struts-deferred.xml, which Dispatcher loads last (after the app config and core's StrutsBeanSelectionProvider), so the alias honors the override. Mirrors the velocity plugin. JSONUtil is unchanged from main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
789dbf3cd2 |
WW-5640 Add WebJars support to Struts core (#1765)
* WW-5640 docs: design for WebJars support in Struts core Adds first-class WebJars support so client-side libraries can be referenced by a version-less logical path and served through the existing static-content pipeline. Grounded against 7.2.x source. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 docs: implementation plan for WebJars support Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 build: add webjars-locator-lite dependency Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 feat: add webjars config constants and defaults Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 docs: correct plan test framework to JUnit 4 core uses JUnit 4 + AssertJ + Mockito, not JUnit 5 Jupiter (no Jupiter engine on the classpath). Test tasks translate accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 feat: add WebJarUrlProvider resolution seam Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 feat: register WebJarUrlProvider bean Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 feat: extend static content-type map for webjar assets Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 feat: serve webjar assets via static content loader Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 feat: add <s:webjar> tag and <@s.webjar> macro Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 docs: add generated tag reference for <s:webjar> Annotation-processor-generated tag reference (attributes + description), tracked like every other tag's docs under core/src/site/resources/tags/. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 fix: address final review (log level, resolveUrl traversal test, javadoc) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 refactor: address SonarCloud code smells - getContentType: replace long if/else chain with a static extension-> MIME map (S3776 cognitive complexity) - DefaultWebJarUrlProvider.split: return Optional<String[]> instead of a null sentinel (S1168; Optional fits the reject semantics, empty-array would not) - serving tests: rename local 'loader' -> 'webJarLoader' to stop hiding the ContentTypeProbe field (S1117) - WebJarTest: use assertThat(writer).hasToString(...) (S5838) S110 (WebJarTag inheritance depth) is inherent to the Struts tag base class hierarchy shared by every tag; left as-is. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
59e342488d |
WW-5256 Decouple FreeMarker whitespace stripping from devMode (#1743)
* WW-5256 docs: design to decouple FreeMarker whitespace stripping from devMode Fixes s:textarea rendering blank lines and HTML whitespace bloat in devMode by honoring struts.freemarker.whitespaceStripping unconditionally. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5256 docs: implementation plan to decouple whitespace stripping from devMode Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5256 test: prove whitespace stripping wrongly disabled in devMode * WW-5256 fix(freemarker): honor whitespaceStripping regardless of devMode * WW-5256 docs: drop devMode note from whitespaceStripping constant --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
4f3fd69aa6 |
WW-5632 Harden commons-fileupload2 dependency against milestone binary-incompatibility (#1735)
* WW-5632 docs: add commons-fileupload2 milestone-hardening design spec Design for hardening the commons-fileupload2 dependency against milestone binary-incompatibility (manage -core, activate a scoped enforcer rule, add a runtime API guard in AbstractMultiPartRequest). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5632 docs: add implementation plan for fileupload2 milestone hardening Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5632 build(deps): manage commons-fileupload2-core alongside jakarta-servlet6 Pin both commons-fileupload2 artifacts to a single commons-fileupload2.version property so the volatile -core API can no longer skew from -jakarta-servlet6 in the reactor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5632 build: enforce a single commons-fileupload2 version Activate maven-enforcer-plugin (previously dormant in pluginManagement) with a fileupload-scoped bannedDependencies rule so any divergent commons-fileupload2 version fails the build early. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5632 fix(fileupload): fail fast on incompatible commons-fileupload2 API Verify once per JVM that the fileupload size-limit setters exist and throw a clear StrutsException reporting the core/jakarta version skew, replacing an opaque deep-stack NoSuchMethodError in downstream runtimes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5632 fix(fileupload): make API-verification guard static Resolve Sonar java:S2696 (instance method writing a static field) by making ensureFileUploadApiVerified() static; verification is JVM-global. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |