Copilot commented on code in PR #925: URL: https://github.com/apache/fesod/pull/925#discussion_r3366900370
########## fesod-sheet/src/main/java/org/apache/fesod/sheet/annotation/write/ExcelView.java: ########## @@ -0,0 +1,65 @@ +/* + * 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 + * + * http://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.fesod.sheet.annotation.write; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Inherited; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; +import org.apache.fesod.sheet.write.builder.AbstractExcelWriterParameterBuilder; + +/** + * Annotation used for indicating view(s) that the property + * that is defined by field annotated is part of. + * <p> + * An example annotation would be: + * <pre> + * @ExcelView(asTypes = BasicView.class) + * // Or + * @ExcelView(asNames = "BasicView") + * </pre> + * which would specify that field annotated would be included + * when processing (writing) Sheet identified by <code>BasicView.class</code> (or its subclass) or + * <code>"BasicView"</code>. + * If multiple View class or string identifiers are included, the field will be part of all of them. + * </p> + * + * @see AbstractExcelWriterParameterBuilder#groups(Class[]) + * @see AbstractExcelWriterParameterBuilder#groups(String[]) + */ +@Target(ElementType.FIELD) +@Retention(RetentionPolicy.RUNTIME) +@Inherited +public @interface ExcelView { Review Comment: `@Inherited` should be removed here as it is not applicable to `ElementType.FIELD` annotations and may cause confusion. ########## fesod-sheet/src/test/java/org/apache/fesod/sheet/view/WriteSheetViewTests.java: ########## @@ -0,0 +1,236 @@ +/* + * 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 + * + * http://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.fesod.sheet.view; + +import java.io.File; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.io.Reader; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import org.apache.commons.csv.CSVFormat; +import org.apache.commons.csv.CSVParser; +import org.apache.commons.io.input.BOMInputStream; +import org.apache.fesod.sheet.FesodSheet; +import org.apache.fesod.sheet.support.ExcelTypeEnum; +import org.apache.poi.ss.usermodel.Row; +import org.apache.poi.ss.usermodel.Sheet; +import org.apache.poi.ss.usermodel.Workbook; +import org.apache.poi.ss.usermodel.WorkbookFactory; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * Tests for the view-based export grouping feature using {@code @ExcelView}. + */ +class WriteSheetViewTests { + + private File write03; + private File write07; + private File writeCsv; + + @BeforeEach + void setUp(@TempDir Path tempDir) { + write03 = createTmpFile(tempDir, "write03.xls"); + write07 = createTmpFile(tempDir, "write07.xls"); + writeCsv = createTmpFile(tempDir, "writeCsv.csv"); Review Comment: The XLSX test file uses a `.xls` extension, which is confusing when debugging failures and can mislead readers about the format being written/read. Use a `.xlsx` filename for the XLSX case. ########## fesod-sheet/src/main/java/org/apache/fesod/sheet/annotation/write/ExcelView.java: ########## @@ -0,0 +1,65 @@ +/* + * 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 + * + * http://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.fesod.sheet.annotation.write; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Inherited; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; +import org.apache.fesod.sheet.write.builder.AbstractExcelWriterParameterBuilder; + +/** + * Annotation used for indicating view(s) that the property + * that is defined by field annotated is part of. + * <p> + * An example annotation would be: + * <pre> + * @ExcelView(asTypes = BasicView.class) + * // Or + * @ExcelView(asNames = "BasicView") + * </pre> + * which would specify that field annotated would be included + * when processing (writing) Sheet identified by <code>BasicView.class</code> (or its subclass) or + * <code>"BasicView"</code>. + * If multiple View class or string identifiers are included, the field will be part of all of them. + * </p> + * + * @see AbstractExcelWriterParameterBuilder#groups(Class[]) + * @see AbstractExcelWriterParameterBuilder#groups(String[]) + */ +@Target(ElementType.FIELD) +@Retention(RetentionPolicy.RUNTIME) +@Inherited +public @interface ExcelView { + + /** + * View or views that annotated element is part of. Views are identified + * by classes, and use expected class inheritance relationship: child + * views contain all elements parent views have. + */ Review Comment: The class-based view inheritance semantics described here ("child views contain all elements parent views have") don’t match the implemented/tested behavior, where selecting a parent view includes fields annotated with its sub-views (see `ClassBasedViewMatcher#isAssignableFrom` and `WriteSheetViewTests#testWriteWithBaseAndSubTypes`). Please align the Javadoc with actual behavior to avoid API confusion. ########## fesod-sheet/src/main/java/org/apache/fesod/sheet/write/view/ClassBasedViewMatcher.java: ########## @@ -0,0 +1,60 @@ +/* + * 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 + * + * http://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.fesod.sheet.write.view; + +import java.lang.reflect.Field; +import java.util.Arrays; +import java.util.Collection; +import java.util.Optional; +import lombok.EqualsAndHashCode; +import lombok.RequiredArgsConstructor; +import org.apache.commons.collections4.CollectionUtils; +import org.apache.commons.lang3.ArrayUtils; +import org.apache.fesod.sheet.annotation.write.ExcelView; + +/** + * View matcher that resolves view-based on class + * identifiers declared in {@code @ExcelView#asTypes()}. + */ +@EqualsAndHashCode +@RequiredArgsConstructor +public class ClassBasedViewMatcher implements WriteViewMatcher { + + private final Collection<Class<?>> expectedGroups; + + @Override + public boolean hasViews() { + return CollectionUtils.isNotEmpty(expectedGroups); + } + + @Override + public boolean matches(Field field) { + Class<?>[] fieldGroups = Optional.ofNullable(field.getAnnotation(ExcelView.class)) + .map(ExcelView::asTypes) + .orElse(new Class<?>[0]); + + if (ArrayUtils.isEmpty(fieldGroups)) { + return false; + } + + return Arrays.stream(fieldGroups).anyMatch(fieldGroup -> expectedGroups.stream() + .anyMatch(expectedGroup -> expectedGroup.isAssignableFrom(fieldGroup))); + } Review Comment: `expectedGroups` can be null (e.g., if a caller passes a null varargs array into the builder), which would cause a `NullPointerException` here. Use a null-safe collection before streaming. ########## fesod-sheet/src/main/java/org/apache/fesod/sheet/write/builder/AbstractExcelWriterParameterBuilder.java: ########## @@ -170,4 +173,26 @@ public T orderByIncludeColumn(Boolean orderByIncludeColumn) { parameter().setOrderByIncludeColumn(orderByIncludeColumn); return self(); } + + /** + * Only write the fields marked by the following View class identifiers. + * + * @param types Target View class identifiers + * @return this + */ + public T groups(Class<?>... types) { + parameter().setWriteViewMatcher(new ClassBasedViewMatcher(Arrays.asList(types))); + return self(); + } Review Comment: Both `groups(...)` overloads throw a `NullPointerException` if the caller passes a null varargs array (e.g., `groups((Class<?>[]) null)` / `groups((String[]) null)`), and `Arrays.asList(...)` keeps a mutable view backed by the caller-provided array. Consider null-guarding and defensively copying to make the builder more robust. ########## fesod-sheet/src/main/java/org/apache/fesod/sheet/write/view/NameBasedViewMatcher.java: ########## @@ -0,0 +1,60 @@ +/* + * 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 + * + * http://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.fesod.sheet.write.view; + +import java.lang.reflect.Field; +import java.util.Arrays; +import java.util.Collection; +import java.util.Optional; +import lombok.EqualsAndHashCode; +import lombok.RequiredArgsConstructor; +import org.apache.commons.collections4.CollectionUtils; +import org.apache.commons.lang3.ArrayUtils; +import org.apache.fesod.sheet.annotation.write.ExcelView; + +/** + * View matcher that resolves view-based on string + * identifiers declared in {@code @ExcelView#asNames()}. + */ +@EqualsAndHashCode +@RequiredArgsConstructor +public class NameBasedViewMatcher implements WriteViewMatcher { + + private final Collection<String> expectedGroups; + + @Override + public boolean hasViews() { + return CollectionUtils.isNotEmpty(expectedGroups); + } + + @Override + public boolean matches(Field field) { + String[] fieldGroups = Optional.ofNullable(field.getAnnotation(ExcelView.class)) + .map(ExcelView::asNames) + .orElse(new String[0]); + + if (ArrayUtils.isEmpty(fieldGroups)) { + return false; + } + + return Arrays.stream(fieldGroups).anyMatch(fieldGroup -> expectedGroups.stream() + .anyMatch(expectedGroup -> expectedGroup.equals(fieldGroup))); + } Review Comment: `expectedGroups` can be null (e.g., if a caller passes a null varargs array into the builder), which would cause a `NullPointerException` here. This can be simplified and made null-safe by using `CollectionUtils.emptyIfNull(expectedGroups)` and `contains`. ########## fesod-sheet/src/main/java/org/apache/fesod/sheet/annotation/write/ExcelView.java: ########## @@ -0,0 +1,65 @@ +/* + * 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 + * + * http://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.fesod.sheet.annotation.write; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Inherited; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; Review Comment: `@Inherited` has no effect on field-targeted annotations (it is only honored for class-level annotations), which can mislead readers into thinking the annotation is inherited on fields. Remove it. ########## fesod-sheet/src/main/java/org/apache/fesod/sheet/write/view/NoopWriteViewMatcher.java: ########## @@ -0,0 +1,42 @@ +/* + * 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 + * + * http://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.fesod.sheet.write.view; + +import java.lang.reflect.Field; +import lombok.EqualsAndHashCode; + +/** + * The default implementation for {@link WriteViewMatcher}. + */ +@EqualsAndHashCode +public class NoopWriteViewMatcher implements WriteViewMatcher { + + public static final NoopWriteViewMatcher INSTANCE = new NoopWriteViewMatcher(); + + @Override + public boolean hasViews() { + return false; + } + + @Override + public boolean matches(Field field) { + return false; + } Review Comment: `NoopWriteViewMatcher` represents the "no view filtering" case, so `matches(...)` returning `false` is counter-intuitive and makes it easy for future call sites to accidentally exclude everything if they forget to check `hasViews()` first. Returning `true` here makes the default behavior safe (include all fields) while `hasViews()` still signals that no constraints are active. ########## fesod-sheet/src/main/java/org/apache/fesod/sheet/write/builder/AbstractExcelWriterParameterBuilder.java: ########## @@ -170,4 +173,26 @@ public T orderByIncludeColumn(Boolean orderByIncludeColumn) { parameter().setOrderByIncludeColumn(orderByIncludeColumn); return self(); } + + /** + * Only write the fields marked by the following View class identifiers. + * + * @param types Target View class identifiers + * @return this + */ + public T groups(Class<?>... types) { + parameter().setWriteViewMatcher(new ClassBasedViewMatcher(Arrays.asList(types))); + return self(); + } + + /** + * Only write to the fields marked by the following View string identifiers. + * + * @param names Target View string identifiers + * @return this + */ + public T groups(String... names) { + parameter().setWriteViewMatcher(new NameBasedViewMatcher(Arrays.asList(names))); + return self(); + } Review Comment: Same robustness issue as the class-based overload: null varargs will NPE, and the list returned by `Arrays.asList(...)` is backed by the provided array. Null-guard and copy to avoid surprises. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
