REPORT-904: Consider client timezone when processing LocalDateTime values

[icrc-psousa]

Ticket: REPORT-904

[icrc-psousa]

The dates retrieved from the resultSet are returned as LocalDateTime instead of Date. Not sure if this happens due to Java or jdbc driver version we're using or even if it may return some other type.

Here, I'm converting LocalDateTime to Date considering the timezone on the request so that field is treated as Date on the report. Not sure if it's the right place or way of doing it. Any advice is appreciated

[ibacher]

Could we add the above to the code?

[mseaton]

I don't think this is the ideal place to put this code. This feels more like something that should be handled at either the time of rendering or the time of evaluation, than something to apply like this as post-processing to an entire report within the service.

If we go with the 2nd option I propose above, which is to have clientTimezone able to be defined as a context value in the evaluation context and overridden by a parameter value in the report request, then the next step would be to create a generic method to render out column values that are of type LocalDate by taking into account the clientTimezone set in the Evaluation Context contextValues.

[icrc-psousa]

Regarding the place to do this conversion, I'm kind of divided on whether doing it at the time of evaluation or rendering.

I assume that, timezone conversion at the time of evaluation, for the SQL, would be done here but I don't know about the other evaluators and scenarios. Conversion should be handled on all evaluators other than SqlDataSetEvaluator, no?

If done at the time of rendering would I assume it would be done on the XlsReportRenderer for the Excel but, again, it would have to be done for all the renders, right?

I don't have a definitive opinion on this. @mseaton @ibacher could you provide more input on your thoughts?

[mseaton]

Great points, and I agree that this is tricky (see my comment above). Probably this does not make the most sense.

[icrc-psousa]

Added new field so that the requester is able to specify the timezone for the dates on the resulting report

[mseaton]

I'm a little wary about this, but I'm not sure I have a much better alternative to offer. The two existing mechanisms that could best support this already on the ReportRequest are:

1. RenderingMode. If we can adapt the RenderingMode argument or otherwise evolve RenderingMode to allow passing in a timezone, we could potentially use this. But I don't think this will be that straightforward based on how RenderingMode was originally designed.

2. Parameters. This might be a reasonable alternative. Basically we could say that any parameter passed in which matches the name of an existing "context value" in the evaluation context would override that context value. Then we could add a new default "context value" named "timezone" which would have a value of the server timezone. And this could be overridden by adding a parameter with the same name to the report. Then, we'd ensure this timezone context value from the evaluation context was used whenever dates are rendered.

If we don't think using Parameters in this way is right, another take on 2 would be to add a new column to ReportRequest, but instead of having it be something very specific named "clientTimezone", we'd make it generic and call it "contextValues" or something, and let it define a serialized map or properties of key/values.

What do you think about the second option above?

[icrc-psousa]

I assume that adding a parameter (2) will be something like, when creating the request:

ReportRequest reportRequest = new ReportRequest();
reportRequest.setReportDefinition(new Mapped<ReportDefinition>(reportDefinition, Map.of("timezone", "Europe/Zurich")));

Then we would override the "timezone" context value here

The default "timezone" would be set here by adding addContextValue("timezone", ZoneId.systemDefault());

This seems a good approach to me. @mseaton do you think we can proceed with using the existing parameters as above or is it worth creating a new column "contextValues" on the ReportRequest?

Let me know if I've got something wrong or if you have any other input.

cc @ibacher

[mseaton]

Sort of. Basically we already have code in a few places in the reporting module that allows context values and parameter values to be used when rendering a report. For example, the ExcelTemplateRenderer runs all values through the EvaluationUtil.evaluateExpression method, which allows values to be converted using a defined syntax (note, we'd need to make a fix here to set the context values first, and then add parameter values, currently this is reversed incorrectly). However, we aren't at all consistent about how these are used (and this goes along with the comment you made below) - we'd have to review all of the different renderers and other ways that report data is output (eg. from the reportingrest module, etc) in order to ensure this was handled consistently, which might be difficult. So this idea probably needs to be reconsidered.

[ibacher]

@mseaton I'd value your input here. This is to support systems that run with UTC dates on the database, but want to generate reports in the client's local time. The idea is just to make the timezone reported a parameter of the query, and otherwise, just handle things normally.

[mseaton]

he idea is just to make the timezone reported a parameter of the query, and otherwise, just handle things normally

It all makes sense. I throw out another idea above in which we maybe just use existing report parameters and context values for this, rather than adding a new column / property. Either way seems reasonable, I just think this will be more flexible.

Either way, I don't think that we should be applying this as a bulk post-processing step on the entire report, but rather need to work it into the evaluators and/or renderers themselves.

[mseaton]

Giving this a little more thought, I do think it's tricky to do what I suggested for the reasons you lay out. I still don't really love ramming this date conversion stuff in unless we can't find a suitable alternative. A few new possible suggestions to consider here:

1. If all of your reports use the SqlFileDataSetDefinition, then you could potentially handle this directly in the SQL query itself. eg. add a "clientTimezone" parameter into your data set definition. This will then become available to your query via a SQL variable named @clientTimezone, and you can then use this in your query like:

select convert_tz(my_date_column,@@session.time_zone, @clientTimezone)

2. If the above suggestion doesn't work, or if you need to handle a range of data set definitions beyond Sql-based DSDs, you could create a new TimezoneConversionDataSetDefinition that you use to wrap / convert all of your existing Data Set Definitions. This would have String timezone and Mapped<DataSetDefinition> wrapped configuration properties, and provide an evaluator implementation that evaluates the wrapped DSD, iterates over it's rows, convert any column values that need converting to the appropriate value based on the timezone parameter, and returns this converted data set.

[mseaton]

Sort of. Basically we already have code in a few places in the reporting module that allows context values and parameter values to be used when rendering a report. For example, the ExcelTemplateRenderer runs all values through the EvaluationUtil.evaluateExpression method, which allows values to be converted using a defined syntax (note, we'd need to make a fix here to set the context values first, and then add parameter values, currently this is reversed incorrectly). However, we aren't at all consistent about how these are used (and this goes along with the comment you made below) - we'd have to review all of the different renderers and other ways that report data is output (eg. from the reportingrest module, etc) in order to ensure this was handled consistently, which might be difficult. So this idea probably needs to be reconsidered.

[mseaton]

Great points, and I agree that this is tricky (see my comment above). Probably this does not make the most sense.

[icrc-psousa]

1. If all of your reports use the SqlFileDataSetDefinition, then you could potentially handle this directly in the SQL query itself. eg. add a "clientTimezone" parameter into your data set definition. This will then become available to your query via a SQL variable named @clientTimezone, and you can then use this in your query like:

select convert_tz(my_date_column,@@session.time_zone, @clientTimezone)

@mseaton This suggestion sounds ok and will cover ICRC needs regarding the timezone issues on the report. I'll try it and I'll then give some feedback.

[icrc-psousa]

Abandoning PR as this was tackled with a different approach by passing the Timezone Offset as a parameter to the report. It has a default value per environment but the user can alter it.