Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The code was previously building a new LocationLevel for each level v… #574

Merged
merged 27 commits into from
May 17, 2024

Conversation

rma-rripken
Copy link
Collaborator

…alue.

I was looking into the NAB reported issue with East Sidney.Elev.Inst.0.Top of Conservation and I got CDA to generate a buggy json response.
response_1710176111087.json

I duplicates the level for each level value in a nasty way.

I think these changes will fix it but I don't have a dataset handy to sufficiently test it locally.

@rma-rripken
Copy link
Collaborator Author

#586

…its currently come back as SI units, better to throw an exception until it gets fixed
…columns. Added CALENDAR_OFFSET to the orderBy clause so that the seasonal values would be returned in order. The Condition changes from the previous commit were incorrect. Because of how the JDom classes work the dao currently only supports SI
…columns. Added CALENDAR_OFFSET to the orderBy clause so that the seasonal values would be returned in order. The Condition changes from the previous commit were incorrect. Because of how the JDom classes work the dao currently only supports SI
unit = UnitSystem.SI.getValue();
}
if(!UnitSystem.SI.getValue().equals(unit)) {
throw new IllegalArgumentException("Levels Version 2 currently only supports SI");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this also support just setting the unit? I swear I corrected some behavior for Ruth to get %, maybe it's a documentation thing though, the levels operate just like the timeseries as the units can be anything not just a length.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getAll is querying against a AV_LOCATION_LEVEL and feeding the rows into the older JDom* classes.
The Dao code for that method is clearly based on:
https://bitbucket.hecdev.net/projects/CWMS/repos/hec-cwms-data-access/browse/hec-db-jdbc/src/main/java/wcds/dbi/oracle/cwms/CwmsLevelJdbcDao.java

It looks to me like your changes were for calls from getOne for a single Level. But maybe there are other pl/sql procs we should call instead of hitting the view.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that structure of this is fine, so we'll go for it for now. But someone is likely going to complain about not being able to set their preferred units.

@rma-rripken rma-rripken marked this pull request as ready for review March 15, 2024 22:46
zchapman-gei and others added 16 commits April 5, 2024 11:22
- Fields, constructors, and getters
- Round trip unit tests
- File parsing unit tests
adding handlers as lambda expressions does not work as the generated anonymous inner class itself isn't annotated
Read from the ctx.body as an InputStream.
Added an png example.
Added url fields
Added methods to get requiredParams and required ZonedDateTimes.  These methods throw an exception if the required param isn't found.  Added exception block to ApiServlet that lets the users know what parameter wasn't found.
Added some tests.
Switching to UnaryOperator and Predicate<ResultSet>.
Including url-template for clobs
Passing contextPath to TextTimeSeriesController in-order to allow controller to build up url to clobs.
Util for doing simple {name} string replacement
…tern annotation. Added versioned date to unit test.
…sistent. Removed overloaded "long" dateTime methods in BinaryTimeSeriesRow Builder. This resolves parsing issue with jackson attempting to parse an Instant string into a long.
…its currently come back as SI units, better to throw an exception until it gets fixed
MikeNeilson
MikeNeilson previously approved these changes May 17, 2024
unit = UnitSystem.SI.getValue();
}
if(!UnitSystem.SI.getValue().equals(unit)) {
throw new IllegalArgumentException("Levels Version 2 currently only supports SI");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that structure of this is fine, so we'll go for it for now. But someone is likely going to complain about not being able to set their preferred units.

@rma-rripken rma-rripken merged commit 23d6ead into USACE:develop May 17, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants