Copilot commented on code in PR #3724:
URL: https://github.com/apache/avro/pull/3724#discussion_r3043359138
##########
lang/js/package-lock.json:
##########
@@ -1,41 +1,61 @@
{
"name": "avro-js",
- "version": "1.12.1",
- "lockfileVersion": 1,
+ "version": "1.12.2-SNAPSHOT",
+ "lockfileVersion": 3,
Review Comment:
lockfileVersion was bumped to 3, which is not supported by older npm
versions (e.g., npm 6 that typically ships with Node 12). Given the workflows
still include Node 12, this can break installs; align Node/npm versions in CI
with this lockfile format (or keep lockfileVersion 1).
```suggestion
"lockfileVersion": 1,
```
##########
.github/workflows/test-lang-js.yml:
##########
@@ -45,13 +45,13 @@ jobs:
- 14
- 16
steps:
- - uses: actions/checkout@v5
+ - uses: actions/checkout@v6
- name: Setup Node
- uses: actions/setup-node@v4
+ uses: actions/setup-node@v6
with:
node-version: ${{ matrix.node }}
Review Comment:
The workflow still tests Node 12/14/16, but lang/js now depends on mocha@11
and nyc@18 (via package-lock) which require newer Node versions (mocha:
>=18.18; nyc: >=20). Update the matrix to supported Node versions (or
revert/pin the tooling versions) so CI doesn’t fail at install/runtime.
##########
lang/java/avro/src/test/java/org/apache/avro/io/TestFastReaderBuilderClassLoading.java:
##########
@@ -0,0 +1,120 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.avro.io;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.net.URI;
+import java.util.Collections;
+
+import org.apache.avro.Schema;
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericDatumReader;
+import org.apache.avro.generic.GenericDatumWriter;
+import org.apache.avro.generic.GenericRecord;
+import org.apache.avro.generic.GenericRecordBuilder;
+import org.apache.avro.specific.SpecificData;
+import org.apache.avro.util.ClassSecurityValidator;
+import org.apache.avro.util.ClassSecurityValidator.ClassSecurityPredicate;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests that FastReaderBuilder.findClass() routes class loading through
+ * ClassUtils.forName(), so that ClassSecurityValidator is applied
consistently.
+ */
+public class TestFastReaderBuilderClassLoading {
+
+ private static final String TEST_VALUE = "https://example.com";;
+
+ private ClassSecurityPredicate originalValidator;
+
+ @BeforeEach
+ public void saveValidator() {
+ originalValidator = ClassSecurityValidator.getGlobal();
+ }
+
+ @AfterEach
+ public void restoreValidator() {
+ ClassSecurityValidator.setGlobal(originalValidator);
+ }
+
+ /**
+ * When the validator blocks a class referenced by java-class,
FastReaderBuilder
+ * must propagate the SecurityException so the caller knows why the class was
+ * rejected.
+ */
+ @Test
+ void blockedClassThrowsSecurityException() {
+ // Block java.net.URI
Review Comment:
The comment says “Block java.net.URI”, but the code is actually configuring
an allow-list that does not include URI (defaults + Utf8). Consider rewording
the comment to match the behavior (e.g., “Do not trust java.net.URI”) to avoid
implying an explicit deny-list.
```suggestion
// Do not trust java.net.URI
```
##########
.github/workflows/test-lang-js.yml:
##########
@@ -76,21 +76,21 @@ jobs:
- 14
- 16
steps:
- - uses: actions/checkout@v5
+ - uses: actions/checkout@v6
- name: Setup Node
- uses: actions/setup-node@v4
+ uses: actions/setup-node@v6
with:
node-version: ${{ matrix.node }}
Review Comment:
Same issue in the interop job: Node 12/14/16 are no longer compatible with
the updated JS dev tooling (mocha@11 / nyc@18). Align the Node matrix with the
minimum supported version (or pin the devDependencies) to keep this workflow
runnable.
##########
lang/js/package-lock.json:
##########
@@ -1685,337 +2082,504 @@
"p-map": "^3.0.0",
"process-on-spawn": "^1.0.0",
"resolve-from": "^5.0.0",
- "rimraf": "^3.0.0",
+ "rimraf": "^6.1.3",
"signal-exit": "^3.0.2",
- "spawn-wrap": "^2.0.0",
- "test-exclude": "^6.0.0",
+ "spawn-wrap": "^3.0.0",
+ "test-exclude": "^8.0.0",
"yargs": "^15.0.2"
},
+ "bin": {
+ "nyc": "bin/nyc.js"
+ },
+ "engines": {
+ "node": "20 || >=22"
+ }
Review Comment:
package-lock indicates nyc@18 requires Node "20 || >=22". With the current
CI matrix (Node 12/14/16), `npm run cover` will break. Either lower nyc/mocha
to versions compatible with the supported Node range, or bump the supported
Node versions everywhere (workflows + package.json engines).
##########
lang/js/package.json:
##########
@@ -45,10 +45,10 @@
"underscore": "^1.13.2"
},
"devDependencies": {
- "nyc": "^17.0.0",
+ "nyc": "^18.0.0",
"coveralls": "^3.1.1",
"jshint": "^2.13.4",
- "mocha": "10.8.2",
+ "mocha": "11.7.5",
"tmp": "^0.2.1"
Review Comment:
Bumping mocha to 11.x and nyc to 18.x effectively drops support for older
Node versions (mocha requires >=18.18; nyc requires >=20 per the lockfile).
Please update the declared Node engine(s) and the GitHub Actions Node matrix
accordingly, or keep the previous toolchain versions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
