fix: Analytics - Excel date export and period picker review EXO-88863 #449
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,13 +20,20 @@ | |
| package io.meeds.analytics.portlet; | ||
|
|
||
| import java.io.IOException; | ||
| import java.math.BigDecimal; | ||
| import java.time.Instant; | ||
| import java.time.LocalDateTime; | ||
| import java.time.ZoneId; | ||
| import java.time.ZoneOffset; | ||
| import java.util.*; | ||
|
|
||
| import javax.portlet.*; | ||
| import javax.ws.rs.core.MediaType; | ||
|
|
||
| import org.apache.commons.lang3.StringUtils; | ||
| import org.apache.poi.ss.usermodel.Cell; | ||
| import org.apache.poi.ss.usermodel.CellStyle; | ||
| import org.apache.poi.ss.usermodel.Workbook; | ||
| import org.json.*; | ||
|
|
||
| import org.exoplatform.commons.utils.CommonsUtils; | ||
|
|
@@ -69,6 +76,15 @@ public abstract class AbstractAnalyticsPortlet<T> extends GenericPortlet { | |
|
|
||
| private static final String EXPORT_EXCEL_OPERATION = "EXPORT_EXCEL"; | ||
|
|
||
| /** | ||
| * Both exports write an OOXML workbook (XSSF, ".xlsx"). Declaring the | ||
| * legacy "application/vnd.ms-excel" type of the binary ".xls" format makes | ||
| * Excel greet the download with a "the file format and the extension don't | ||
| * match" warning before opening it. | ||
| */ | ||
| protected static final String XLSX_CONTENT_TYPE = | ||
| "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet"; | ||
|
|
||
| private SpaceService spaceService; | ||
|
|
||
| private AnalyticsService analyticsService; | ||
|
|
@@ -397,6 +413,134 @@ private T clone(T filter) { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Excel number format per date-histogram interval, for the intervals a | ||
| * spreadsheet can render faithfully from a real date value. | ||
| * <p> | ||
| * The day format is Excel's builtin "m/d/yy" (format index 14), which | ||
| * Excel renders using the *reader's* own short-date convention rather | ||
| * than the literal pattern, so a French and an English reader each see | ||
| * their own. The coarser ones spell out a pattern because no locale-aware | ||
| * builtin exists for them. | ||
| * <p> | ||
| * Absent on purpose: quarter and ISO-week (no faithful spreadsheet format | ||
| * token — a real date value would display as its first day, losing the | ||
| * "Q3 2026" / "W37-2026" the chart shows) and hour, whose bucket key is an | ||
| * hour of day (0-23) cumulated over the period, not an instant. Those keep | ||
| * the textual label. | ||
| */ | ||
| private static final Map<String, String> EXCEL_DATE_FORMATS = Map.of(AnalyticsAggregation.YEAR_INTERVAL, | ||
| "yyyy", | ||
| AnalyticsAggregation.MONTH_INTERVAL, | ||
| "mmm yyyy", | ||
| AnalyticsAggregation.DAY_INTERVAL, | ||
| "m/d/yy", | ||
| AnalyticsAggregation.MINUTE_INTERVAL, | ||
| "yyyy-mm-dd hh:mm", | ||
| AnalyticsAggregation.SECOND_INTERVAL, | ||
| "yyyy-mm-dd hh:mm:ss"); | ||
|
|
||
| /** | ||
| * Writes an epoch-milliseconds value as a real date-time cell. | ||
| * <p> | ||
| * Used for a column aggregating a date *field* (a MAX over a "last | ||
| * connection" field, say): the aggregation type is MAX, not DATE, so it is | ||
| * not a date histogram and has no interval — but its value is still an | ||
| * instant, and written as a plain number it reaches the reader as | ||
| * 1.75941E+12. | ||
| * | ||
| * @return {@code true} when the cell was written as a date, {@code false} | ||
| * when the value is not epoch millis and the caller should fall | ||
| * back | ||
| */ | ||
| protected boolean writeTimestampCell(Cell cell, String value, ZoneId zoneId, Map<String, CellStyle> styles) { | ||
|
Member
There was a problem hiding this comment. 🟠 High — a MIN/MAX over a date field never reaches the reader as a date: the table path is missing the This is the case The two paths that read an aggregation's value diverge. The chart path converts the scientific-notation form: Object value = valueResult.get(VALUE_PARAM);
if (value instanceof BigDecimal bd) {
result = bd.toPlainString(); // "1759410000000"
} else {
result = value.toString();
}The table path does not: value = bucket.getJSONObject(AGGREGATION_RESULT_VALUE_PARAM).get(VALUE_PARAM);
...
itemValue.setValue(toString(value)); // private String toString(Object v) { return Objects.toString(v, null); }Verified against the engines rather than from memory. A real "max_ts":{"value":1.759410123456E12,"value_as_string":"1759410123456"},
"min_ts":{"value":1.75941E12,"value_as_string":"1759410000000"},
"sum_ts":{"value":3.518820123456E12,"value_as_string":"3518820123456"}and the same literals appear in the nested terms-bucket shape this portlet actually builds. Feeding that So
This defect predates this round and was not raised in Round #1 — fixing it here or splitting it into its own task is a scope call for the Architect, not for this review. Fix (hypothesis to confirm — the two options are not equivalent): mirroring the chart path (
Member
Author
There was a problem hiding this comment. Confirmed and fixed in So the failure is exactly as you describe, and it explains something I had wrong: the reporter saw Fixed with the On your two options, I took neither literally. Mutation-verified: restoring |
||
| long timestamp; | ||
| try { | ||
| // Parsed as a decimal, not with Long.parseLong: a metric aggregation's | ||
| // value arrives from Elasticsearch as a JSON floating-point literal and | ||
| // org.json turns it into a BigDecimal, whose toString is scientific | ||
| // notation ("1.75941E+12"). Long.parseLong rejects that, which left the | ||
| // one case this method exists for - a MIN/MAX over a date field - | ||
| // falling through to a plain number in the reader's spreadsheet. | ||
| timestamp = new BigDecimal(StringUtils.trim(value)).longValueExact(); | ||
| } catch (NumberFormatException | ArithmeticException e) { | ||
| // Not a whole number of milliseconds: not an instant | ||
| return false; | ||
| } | ||
| if (timestamp <= 0) { | ||
| // A "never connected" style zero is not a date, and would export as | ||
| // 1 January 1970 | ||
| return false; | ||
| } | ||
| writeDateValue(cell, timestamp, zoneId, styles, "yyyy-mm-dd hh:mm"); | ||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Writes a date bucket as a real date-typed cell instead of the localized | ||
| * label the chart displays. | ||
| * <p> | ||
| * A label such as "1 sept. 2026" written as text is only a picture of a | ||
| * date to a spreadsheet: it cannot be sorted chronologically (it sorts | ||
| * lexicographically, so "10 août" lands before "1 sept."), filtered by | ||
| * period, or fed to a date formula, and no cell formatting recovers it | ||
| * because the underlying value is a string. A date-typed cell carries the | ||
| * instant itself and each reader's Excel renders it in their own locale. | ||
| * | ||
| * @param cell cell to write | ||
| * @param aggregation the aggregation the bucket belongs to | ||
| * @param key the raw bucket key, epoch milliseconds for a date | ||
| * histogram | ||
| * @param zoneId time zone the buckets were aligned on, so the written | ||
| * wall-clock date is the one the chart shows | ||
| * @param styles per-workbook cache of the created cell styles: a | ||
| * workbook holds a bounded number of them, so one per | ||
| * cell would both bloat the file and eventually hit | ||
| * that limit | ||
| * @return {@code true} when the cell was written as a date, {@code false} | ||
| * when this bucket has no faithful date representation and the | ||
| * caller should fall back to the textual label | ||
| */ | ||
| protected boolean writeDateCell(Cell cell, | ||
| AnalyticsAggregation aggregation, | ||
| String key, | ||
| ZoneId zoneId, | ||
| Map<String, CellStyle> styles) { | ||
| if (aggregation == null || StringUtils.isBlank(key)) { | ||
| return false; | ||
| } | ||
| String excelFormat = EXCEL_DATE_FORMATS.get(aggregation.getInterval()); | ||
| if (excelFormat == null) { | ||
| return false; | ||
| } | ||
| long timestamp; | ||
| try { | ||
| timestamp = Long.parseLong(key); | ||
| } catch (NumberFormatException e) { | ||
| // Not an epoch-millis bucket key after all: the textual label is the | ||
| // only representation left | ||
| LOG.debug("Analytics export: bucket key '{}' is not a timestamp, exporting its label instead", key, e); | ||
| return false; | ||
| } | ||
| writeDateValue(cell, timestamp, zoneId, styles, excelFormat); | ||
| return true; | ||
| } | ||
|
|
||
| private void writeDateValue(Cell cell, long timestamp, ZoneId zoneId, Map<String, CellStyle> styles, String excelFormat) { | ||
| Workbook workbook = cell.getSheet().getWorkbook(); | ||
| CellStyle style = styles.computeIfAbsent(excelFormat, format -> { | ||
| CellStyle createdStyle = workbook.createCellStyle(); | ||
| createdStyle.setDataFormat(workbook.createDataFormat().getFormat(format)); | ||
| return createdStyle; | ||
| }); | ||
| // setCellValue(LocalDateTime) writes the wall-clock value as-is, unlike | ||
| // the Date overload which would re-read it through the server's default | ||
| // time zone | ||
| cell.setCellValue(LocalDateTime.ofInstant(Instant.ofEpochMilli(timestamp), | ||
| zoneId == null ? ZoneOffset.UTC : zoneId)); | ||
| cell.setCellStyle(style); | ||
| } | ||
|
|
||
| enum SearchScope { | ||
| USER, | ||
| SPACE, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.