Improve yaml syntax: Use collapsed for suitable classes (Breaking Change)#2776
Conversation
📝 WalkthroughWalkthroughAdded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/FileUserDataProvider.java (2)
30-43:⚠️ Potential issue | 🟡 MinorJavadoc still references the old element name
fileUserDataProvider.Line 33 in the
@descriptionblock still mentionsfileUserDataProvider, but the element was renamed tohtpasswdFileProvideron line 43. Update the documentation to match the new name.Proposed fix
- * The <i>fileUserDataProvider</i> loads users from a file in the format + * The <i>htpasswdFileProvider</i> loads users from a file in the format
108-115:⚠️ Potential issue | 🟠 MajorPotential
NullPointerExceptioniflocationis not set beforeinit()is called.
Paths.get(this.location)will throwNullPointerExceptioniflocationisnull. Consider adding a null/empty check with a descriptive error message, or annotatingsetLocationwith@Required.Proposed fix
public void init(Router router) { + if (location == null || location.isBlank()) { + throw new IllegalStateException("'location' must be set on htpasswdFileProvider"); + } List<String> lines; try { lines = Files.readAllLines(Paths.get(this.location));
🤖 Fix all issues with AI agents
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/FileUserDataProvider.java`:
- Around line 43-57: The MCElement name was changed from "fileUserDataProvider"
to "htpasswdFileProvider" and the attribute from "htpasswdPath" to "location"
but docs, samples, tests and Javadoc were not updated; update
docs/MIGRATION-GUIDE.md to list this breaking change and the exact rename,
replace the old element/attribute names in the sample configs
(distribution/examples/security/basic-auth/simple/proxies.xml, apis.yaml,
README.md and root README.md), update YAMLComponentsParsingTest
(annot/src/test/java/com/predic8/membrane/annot/YAMLComponentsParsingTest.java)
to expect "htpasswdFileProvider" and "location", and update the Javadoc in
FileUserDataProvider (class FileUserDataProvider, setLocation/getLocation) to
reference the new names so all examples, tests and docs consistently use
htpasswdFileProvider/location.
|
This pull request needs "/ok-to-test" from an authorized committer. |
|
/ok-to-test |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/CountInterceptor.java`:
- Line 33: Update the Javadoc YAML example in CountInterceptor to use
collapsed-mode syntax by removing the redundant attribute name; change the
example that currently shows `counter: name: Mock Node 1` to the collapsed form
`counter: Mock Node 1` so the YAML sets the counter's name instead of parsing
the whole string; follow the pattern used in YAMLParsingCollapsedTest.java and
ensure the example is updated near the CountInterceptor class documentation.
In `@distribution/examples/security/basic-auth/simple/README.md`:
- Line 54: Update the README sentence that references htpasswdFileProvider to
use a hyphenated compound modifier: change "htpasswd formatted file" to
"htpasswd-formatted file" so the sentence reads e.g. "the htpasswdFileProvider
can be used to read basic authentication data from an htpasswd-formatted file";
keep the rest of the sentence unchanged.
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/FileUserDataProvider.java (1)
52-55: Add@RequiredtosetLocationto fail fast with a clear error.Without
@Required, if a user omits thelocationattribute,init()will throw aNullPointerExceptionatPaths.get(this.location)(line 112) with no indication of which configuration attribute is missing. This pattern is consistently used throughout the session provider classes (LDAPUserDataProvider,JdbcUserDataProvider,EmailTokenProvider, etc.) for required configuration attributes.Proposed fix
+ `@Required` `@MCAttribute` public void setLocation(String path) { this.location = path; }Add the import:
import com.predic8.membrane.annot.Required;
2 participants
Summary by CodeRabbit
Configuration
Style
Documentation
Chores