Feature/time series profile dto #733
Conversation
|
@rma-psmorris please review |
|
@AndreasChristmann this should be flagged as a draft until your work in completed |
| public void validate() throws FieldException | ||
| { | ||
| if (this.parameterList == null) { | ||
| throw new FieldException("Parameter list field can't be null"); |
There was a problem hiding this comment.
FYI, there are more specific exceptions derived from this exception that can be used.
There was a problem hiding this comment.
changed to use the Validator
| @@ -0,0 +1,23 @@ | |||
| { | |||
| "office-id": "SWT", | |||
| "location-id": "TIMESERIESPROFILE_LOC", | |||
There was a problem hiding this comment.
If I remember correctly a base location has a max length of 24 characters. (I didn't count, it just looks long.)
There was a problem hiding this comment.
picked a shorter name
| @Schema(description = "Key Parameter") | ||
| private final String keyParameter; | ||
| @Schema(description = "Reference TS") | ||
| private final String refTsId; |
There was a problem hiding this comment.
Can a time series identifier DTO class be used here?
There was a problem hiding this comment.
@AndreasChristmann reply?
There was a problem hiding this comment.
Is there a time series identifier DTO class? Did not see one. How should it look like if we create one
There was a problem hiding this comment.
That would be a CwmsId
| @JsonNaming(PropertyNamingStrategies.KebabCaseStrategy.class) | ||
| public final class TimeSeriesProfile extends CwmsDTO | ||
| { | ||
| @Schema(description = "Location ID") |
There was a problem hiding this comment.
This should be the location identifier class
There was a problem hiding this comment.
@AndreasChristmann reply?
| } | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
Are you using hashcode/equals in your production code or only in unit testing? If only in unit testing, then this business should be removed from the DTO and placed in test.
| private final String locationId; | ||
| @Schema(description = "Description") | ||
| private final String description; | ||
| @Schema(description = "Parameter List") |
There was a problem hiding this comment.
Did you consider re-using ParameterInfo for the key parameter and the list of parameters?
There was a problem hiding this comment.
@AndreasChristmann reply?
There was a problem hiding this comment.
no, ParameterInfo is for the parser and holds info how to parse.
| @JsonDeserialize(builder = ParameterInfo.Builder.class) | ||
| @JsonInclude(JsonInclude.Include.NON_NULL) | ||
| @JsonNaming(PropertyNamingStrategies.KebabCaseStrategy.class) | ||
| public class ParameterInfo implements CwmsDTOBase |
There was a problem hiding this comment.
Also - what format are you using for this source code, is it using the format that has been created for this repository?
There was a problem hiding this comment.
using the rep formatter now. Class can be final, can make the other Profile classes final too
| { | ||
| return keyParameter; | ||
| } | ||
| public String getRecordDelimiter(){ return String.valueOf(recordDelimiter); } |
There was a problem hiding this comment.
Why hold these as Char in the DTO when your public API has converts them to Strings?
There was a problem hiding this comment.
changed the API to return char
There was a problem hiding this comment.
I meant char, it is not a String, but a single character. Also FieldDelimiter is a Character, as is can be null
|
|
||
| public String getTimeInTwoFields() | ||
| { | ||
| return timeInTwoFields?"T":"F"; |
There was a problem hiding this comment.
This doesnt follow established DTO Boolean handling present elsewhere in the CDA API
There was a problem hiding this comment.
changed to return boolean
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { |
There was a problem hiding this comment.
If not used in production, move to test
| return result; | ||
| } | ||
| @Override | ||
| public boolean equals(Object o) { |
There was a problem hiding this comment.
If not used in production, move to test
There was a problem hiding this comment.
moved equals and hash to test
| @JsonNaming(PropertyNamingStrategies.KebabCaseStrategy.class) | ||
| public class ParameterInfo implements CwmsDTOBase | ||
| { | ||
| private final String parameter; |
There was a problem hiding this comment.
The following columns are in the DB:
LOCATION_CODE,
KEY_PARAMETER_CODE,
PARAMETER_CODE,
PARAMETER_UNIT,
PARAMETER_FIELD,
PARAMETER_COL_START,
PARAMETER_COL_END
There was a problem hiding this comment.
Are you planning on updating to the full set of DB columns?
There was a problem hiding this comment.
yes. now that I know what col_start and col_end is used for
There was a problem hiding this comment.
added colStart and colEnd, note that those are exclusive to Field
|
|
||
| public List<TimeSeriesProfile> retrieveTimeSeriesProfiles(String locationIdMask, String parameterIdMask, String officeIdMask) { | ||
| return connectionResult(dsl, conn -> { | ||
| List<TimeSeriesProfile> timeSeriesProfileList = new ArrayList<>(); |
There was a problem hiding this comment.
Are we sure this isn't going to end up large in the future?
If so we can wait on pagination but it's generally something we want to consider.
There was a problem hiding this comment.
@AndreasChristmann From internal meeting recommend coding this against the view to allow for pagination to be added if needed later on.
| { | ||
| // TODO | ||
| return connectionResult(dsl, conn -> { |
There was a problem hiding this comment.
Same comment as previous about possible need for pagination.
There was a problem hiding this comment.
Similar as above: @AndreasChristmann - From an internal meeting recommend coding this against the view to allow for pagination to be added if needed later on.
|
|
||
| import java.time.Instant; | ||
|
|
||
| @FormattableWith(contentType = Formats.JSONV2, formatter = JsonV2.class) |
There was a problem hiding this comment.
Is this ever formatted on it's own or always as part of some container object? If always within a container it doesn't need the FormattableWith annotation.
There was a problem hiding this comment.
yes it is always part of a container
There was a problem hiding this comment.
removed the annotation
| @Test | ||
| void testCopyTimeSeriesProfile() throws SQLException { | ||
| CwmsDatabaseContainer<?> databaseLink = CwmsDataApiSetupCallback.getDatabaseLink(); | ||
| databaseLink.connection(c -> { |
There was a problem hiding this comment.
all of these usages need to have CwmsDataApiSetupCallback.getWebUser() added as the second parameter so the correct user with appropriate office permissions is used during the operations.
There was a problem hiding this comment.
added getWebUser as the second parameter throughout
| } | ||
|
|
||
| private static TimeSeriesProfile buildTestTimeSeriesProfile(String location, String parameter) { | ||
| String officeId = "HEC"; |
There was a problem hiding this comment.
Use a real office, add it to the user.sql in the test resources.
There was a problem hiding this comment.
now when using getWebUser, I have access to a real office. Switched to LRL
| { | ||
| "parameter": "Temp-Water", | ||
| "parameter": "Temperature", |
There was a problem hiding this comment.
? The original was correct. Temperature is not a base parameter in CWMS.... or have I gone mad?
There was a problem hiding this comment.
switched to Temp-Water again
| } | ||
| ] | ||
| }, | ||
| { | ||
| "parameter": "Depth", | ||
| "unit": "ft", | ||
| "time-value-pairs": | ||
| "time-zone": "PST", | ||
| "time-value-pair-list": |
There was a problem hiding this comment.
Look at the time series "container" from that end point, the naming here should match.
There was a problem hiding this comment.
changed time-value-pair-list to values
| } | ||
| ] | ||
| }, | ||
| { | ||
| "parameter": "Depth", | ||
| "unit": "ft", | ||
| "time-value-pairs": | ||
| "time-zone": "PST", |
There was a problem hiding this comment.
NOTE: if this file is an example of input, CDA should and will only process numeric date-times as UTC.
ISO format data-times should include include their timezone in each but allowing a TZ parameter to use to then immediately convert to UTC.
CDA is pedantic about time and fosters no confusion. (Granted, that's the intent, we've had plenty of confusion.)
There was a problem hiding this comment.
time in the time series container is just a long, output from the instant, I followed the other timeseries container's format.
| "location-id": { | ||
| "office-id": "SWT", | ||
| "name": "location" | ||
| }, | ||
| "key-parameter": "Depth", | ||
| "record-delimiter": "\n", | ||
| "field-delimiter": ",", | ||
| "time-format": "MM/DD/YYYY,HH24:MI:SS", | ||
| "time-zone": "UTC", |
There was a problem hiding this comment.
Given this is to process data coming from not CDA, the previous comments don't necessarily apply.
| "location-id": "TIMESERIESPROFILE_LOC", | ||
| "location-id": { | ||
| "office-id": "SWT", | ||
| "name": "location" |
There was a problem hiding this comment.
Use a more realistic name.
Also since you've used SWT here, use that for the others. Since these sample json file can make up an consistent whole, they should.
There was a problem hiding this comment.
used SWT and real life name
…rofile parser columnar data
| @Override | ||
| public void validate() throws FieldException { | ||
|
|
||
| if(index==null && !(startColumn!=null && endColumn!=null)) |
There was a problem hiding this comment.
I may need to pull down my logic design book but I'm pretty sure the second check is a perfect application for the "exclusive or"
There was a problem hiding this comment.
But let someone else besides me decide if the logic makes sense. Doing too much boolean algebra homework makes you overthink these things.
There was a problem hiding this comment.
my intention was to mimic in the expression that if index is null, then start and end must not be null,
There was a problem hiding this comment.
Oh yeah, the intention is at least clear, just very... nested.
Though I just roughed out a truth table and a clearer way didn't immediately present itself. I think xor would work in aggregate but you wouldn't be able to have as clear of an error message as you currently do. So just ignore me on this one.
…p' into feature/TimeSeriesProfileDto
| @JsonDeserialize(builder = ParameterInfo.Builder.class) | ||
| @JsonInclude(JsonInclude.Include.NON_NULL) | ||
| @JsonNaming(PropertyNamingStrategies.KebabCaseStrategy.class) | ||
| public final class ParameterInfo implements CwmsDTOBase { |
There was a problem hiding this comment.
This class needs a unit test with JSON resource file content
| public void validate() throws FieldException { | ||
| if(index==null && !(startColumn!=null && endColumn!=null)) | ||
| { | ||
| throw new FieldException("if index is null, startColumn and endColumn must be defined!"); |
There was a problem hiding this comment.
Exceptions as a rule need to be dynamic with the content that they are reporting an error on.
"if index is null, startColumn and endColumn must be defined!"
This message does not do enough to help debug the content that caused this issue.
Also use proper sentence case for exception messages, they should be clear and formatted for the end user.
There was a problem hiding this comment.
removed validate, no longer needed
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { |
There was a problem hiding this comment.
What do you need hashCode & equals for?
There was a problem hiding this comment.
removed hashCode and equals
| @JsonDeserialize(builder = TimeValuePair.Builder.class) | ||
| @JsonInclude(JsonInclude.Include.NON_NULL) | ||
| @JsonNaming(PropertyNamingStrategies.KebabCaseStrategy.class) | ||
| public final class TimeValuePair { |
There was a problem hiding this comment.
The database API only gives you time and value, no other tsv data such as quality, DED, version date?
There was a problem hiding this comment.
Reviewed PL/SQL API, pvq_t returns
value binary_double,
quality_code integer);
We need to add quality.
DED and version date are not returned by the pl/sql package API
|
|
||
| public List<TimeSeriesProfile> retrieveTimeSeriesProfiles(String locationIdMask, String parameterIdMask, String officeIdMask) { | ||
| return connectionResult(dsl, conn -> { | ||
| List<TimeSeriesProfile> timeSeriesProfileList = new ArrayList<>(); |
There was a problem hiding this comment.
@AndreasChristmann From internal meeting recommend coding this against the view to allow for pagination to be added if needed later on.
| @Schema(description = "Key Parameter") | ||
| private final String keyParameter; | ||
| @Schema(description = "Reference TS") | ||
| private final String refTsId; |
There was a problem hiding this comment.
That would be a CwmsId
| import cwms.cda.data.dto.CwmsDTOBase; | ||
|
|
||
| @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type") | ||
| @JsonSubTypes({@JsonSubTypes.Type(value = ParameterInfoIndexed.class), |
There was a problem hiding this comment.
Please define the type name parts here following our easy to ready dash-delimited naming pattern, "indexed-parameter-info" (I think this is easiest to read) or "parameter-info-indexed" if you want to adhere to the classname.
There was a problem hiding this comment.
Example:
@JsonSubTypes({@JsonSubTypes.Type(value = TurbineSetting.class, name = "turbine-setting")})
There was a problem hiding this comment.
used name = "indexed-parameter-info"
| validator.required(endColumn, "endColumn"); | ||
| } | ||
|
|
||
| public String parameterInfoString() |
There was a problem hiding this comment.
Why not follow the bean spec for this method?
There was a problem hiding this comment.
bean spec would be: getParameterInfoString()
You need to include an annotation to have Jackson ignore this.
There was a problem hiding this comment.
used getParameterInfoString with JsonIgnore
| } | ||
|
|
||
| @Override | ||
| public String parameterInfoString() |
There was a problem hiding this comment.
Same question as previous on bean spec
There was a problem hiding this comment.
renamed to getParameterInfoString with JsonIgnore
| } | ||
|
|
||
|
|
||
| public BigInteger getTimeField() { |
There was a problem hiding this comment.
This could be Optional, could also include annotations describing that it can be null. Let's discuss during internal meeting.
There was a problem hiding this comment.
timeField must not be null for this class
| } | ||
| catch(cwms.cda.api.errors.NotFoundException ex) | ||
| { | ||
| // return null for not found |
There was a problem hiding this comment.
@zack-rma -- this should include a logger at developer levels
No description provided.