-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add collectionName
and databaseName
attributes to MongoDbProvider
#3467
Conversation
@vy I went ahead and did a new PR to merge with 2.x branch because the classes, projects, and packages already had changes in the 3.x stuff that did seem to make sense to back-port. So tried to just back-port the specific code for this change and leave the rest alone. |
Thanks Josh. I was about to suggest you creating separate PRs for 2.x and
main, but you were already ahead of me. 💯 I will review this one on Monday.
Op za 15 feb 2025 om 06:22 schreef Josh Smith ***@***.***>
… @vy <https://github.com/vy> I went ahead and did a new PR to merge with
2.x branch because the classes, projects, and packages already had changes
in the 3.x stuff that did seem to make sense to back-port. So tried to just
back-port the specific code for this change and leave the rest alone.
—
Reply to this email directly, view it on GitHub
<#3467 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAARTSJCAMTPONE3IN4IOIT2P3FLFAVCNFSM6AAAAABXF6KYPWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNRQG42DSMJUGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
[image: jesmith17]*jesmith17* left a comment (apache/logging-log4j2#3467)
<#3467 (comment)>
@vy <https://github.com/vy> I went ahead and did a new PR to merge with
2.x branch because the classes, projects, and packages already had changes
in the 3.x stuff that did seem to make sense to back-port. So tried to just
back-port the specific code for this change and leave the rest alone.
—
Reply to this email directly, view it on GitHub
<#3467 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAARTSJCAMTPONE3IN4IOIT2P3FLFAVCNFSM6AAAAABXF6KYPWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNRQG42DSMJUGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
This PR introduces improvements to `HttpAppender` and adds a new test class, `HttpAppenderBuilderTest`, to enhance test coverage. The changes include: * Updating `HttpAppender` to improve validating behavior. * Adding HttpAppenderBuilderTest.java to verify the builder logic for HttpAppender. Ensuring that missing configurations (e.g., URL, Layout) correctly log errors. Co-authored-by: Piotr P. Karwasz <[email protected]>
collectionName
and databaseName
attributes to MongoDbProvider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jesmith17, pushed some minor fixes and dropped some remarks. I'd appreciate it if you can address them.
log4j-mongodb4/src/main/java/org/apache/logging/log4j/mongodb4/MongoDb4Connection.java
Outdated
Show resolved
Hide resolved
log4j-mongodb4/src/main/java/org/apache/logging/log4j/mongodb4/MongoDb4Provider.java
Outdated
Show resolved
Hide resolved
log4j-mongodb4/src/main/java/org/apache/logging/log4j/mongodb4/MongoDb4Connection.java
Outdated
Show resolved
Hide resolved
We use mvnw both locally and in CI over several platforms, including macOS.
Could you make sure you have a clean checkout of the source repository and
if it is still failing, share the output, please?
Op di 18 feb 2025 om 14:45 schreef Josh Smith ***@***.***>
… ***@***.**** commented on this pull request.
------------------------------
In
log4j-mongodb4/src/main/java/org/apache/logging/log4j/mongodb4/MongoDb4Connection.java
<#3467 (comment)>
:
> @@ -67,21 +67,23 @@ public MongoDb4Connection(
final ConnectionString connectionString,
final MongoClient mongoClient,
final MongoDatabase mongoDatabase,
+ final String collectionName,
So the first problem is that the maven-wrapper breaks when running it on
my machine using maven 3.9.9
Specifically it complains about needing to update the
distributionSha256Sum which is impossible because 3.9.9. comes with a
sha-512 sum. In order to get any of the ./mvnw commands to run I had to
rebuild the wrapper, but I suspect you don't want me to check that in.
Seems like there is an undocumented dependent on a specific version of
maven that needs to be addressed.
—
Reply to this email directly, view it on GitHub
<#3467 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAARTSLKCB7CV5OKYNPRZX32QM2PZAVCNFSM6AAAAABXF6KYPWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMMRTGYZDIOBRHE>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
You’re right, we can and should indeed improve on this.
Op di 18 feb 2025 om 14:24 schreef Josh Smith ***@***.***>
… ***@***.**** commented on this pull request.
------------------------------
In
log4j-mongodb4/src/main/java/org/apache/logging/log4j/mongodb4/MongoDb4Provider.java
<#3467 (comment)>
:
> + ConnectionString connectionString;
+ try {
+ connectionString = new ConnectionString(connectionStringSource);
+ } catch (final IllegalArgumentException e) {
+ LOGGER.error("Invalid MongoDB connection string `{}`.", connectionStringSource, e);
+ return null;
+ }
+
+ String effectiveDatabaseName = databaseName != null ? databaseName : connectionString.getDatabase();
+ String effectiveCollectionName = collectionName != null ? collectionName : connectionString.getCollection();
+ // Validate the provided databaseName property
+ try {
+ MongoNamespace.checkDatabaseNameValidity(effectiveDatabaseName);
+ } catch (final IllegalArgumentException e) {
+ LOGGER.error("Invalid MongoDB database name `{}`.", effectiveDatabaseName, e);
+ return null;
+ }
+ // Validate the provided collectionName property
+ try {
+ MongoNamespace.checkCollectionNameValidity(effectiveCollectionName);
+ } catch (final IllegalArgumentException e) {
+ LOGGER.error("Invalid MongoDB collection name `{}`.", effectiveCollectionName, e);
+ return null;
+ }
For people just trying to figure out how to add functionality its really
hard when disputes about coding approach between maintainers are getting
done inside the comments for a specific PR. I would recommend that this
becomes something you document better to make this easier for others.
—
Reply to this email directly, view it on GitHub
<#3467 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAARTSKCPLWIUKTK5FFEMZT2QMYBJAVCNFSM6AAAAABXF6KYPWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMMRTGU2TSNJZGA>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
@vy Just did a new clone of the full repo, into a new directory. Verified that it's the 2.x branch. Running
even trying to run a command like In trying to see if I could manually view the sha256 for the 3.9.8 download, it appears to also only offer a sha512. Here is the content of my maven-wrapper.properties file included with the checkout. Ommitting the comments at the top for brevity.
|
Try removing the |
@ppkarwasz
|
By |
* Publish build scans to develocity.apache.org * Use `DEVELOCITY_ACCESS_KEY` to authenticate to `develocity.apache.org`
…j-parent (apache#3452) * Bump org.apache.logging:logging-parent in /log4j-parent Bumps [org.apache.logging:logging-parent](https://github.com/apache/logging-parent) from 11.3.0 to 12.0.0. - [Release notes](https://github.com/apache/logging-parent/releases) - [Commits](apache/logging-parent@rel/11.3.0...rel/12.0.0) --- updated-dependencies: - dependency-name: org.apache.logging:logging-parent dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> * Necessary fixes for `12.0.0` upgrade. Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Piotr P. Karwasz <[email protected]>
This PR starts a separate `verify-reproducibility` job, whenever a snapshot or release is deployed.
This PR starts an `integration-test` job, whenever a snapshot or release is deployed.
Adds a `.logging-parent-bom-activator` file to activate the `bom` profile.
@ppkarwasz There is no But even if that was the offending file then deleting the one in the code base wouldn't have trigger an error about the file missing as it would have found the file in my local home directory, correct? |
Thanks so much Josh for bearing with us to deliver this feature. There are
some last mile wrinkles to be ironed out in the tests, I will take care of
them. (Please keep the PR open, I will continue working on it.)
Op wo 19 feb 2025 om 17:59 schreef Josh Smith ***@***.***>
… ***@***.**** commented on this pull request.
------------------------------
In
log4j-mongodb4/src/main/java/org/apache/logging/log4j/mongodb4/MongoDb4Connection.java
<#3467 (comment)>
:
> @@ -67,21 +67,23 @@ public MongoDb4Connection(
final ConnectionString connectionString,
final MongoClient mongoClient,
final MongoDatabase mongoDatabase,
+ final String collectionName,
Trying the VERBOSE command above produced this
***@***.*** log4j-temp % MVNW_VERBOSE=true ./mvnw validate -N
Couldn't find MAVEN_HOME, downloading and installing it ...
Downloading from: https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.9.8/apache-maven-3.9.8-bin.zip
Downloading to: /var/folders/wm/96zyw_3x4cdbvycjl7btg8sc0000gn/T/tmp.4xLeCVRCI2/apache-maven-3.9.8-bin.zip
Found curl ... using curl
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 8979k 100 8979k 0 0 1584k 0 0:00:05 0:00:05 --:--:-- 1402k
Error: Failed to validate Maven distribution SHA-256, your Maven distribution might be compromised.
If you updated your Maven version, you need to update the specified distributionSha256Sum property.
***@***.*** log4j-temp %
that's after doing a git reset to bring back the deleted
maven-wrapper.properties file.
I suspect that this needs to be moved to an issue so we can separate the
resolution of this from the approval of the code changes for the appender.
—
Reply to this email directly, view it on GitHub
<#3467 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAARTSMOWNLI4JFKYGYHUJ32QSZ7DAVCNFSM6AAAAABXF6KYPWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMMRXGQ2DGMBRGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
…r` (#3467) Co-authored-by: Josh Smith <[email protected]>
Couldn't merge due to missing signatures – squashed and committed in 07590bc. |
Ports #3322 to
2.x