Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Store layer metadata #401
Store layer metadata #401
Changes from 24 commits
1a26063
c27cf13
9152b17
754708a
be6d15b
caa8ab7
03faae3
be61a63
8cc797e
cffe53d
a1d8ec3
79d71d5
da373cc
38319b7
8d2afae
91e2483
71b50cb
1fdf9b6
c719e01
eb6b6d4
5026730
526bbe0
f51e22a
7a27a30
f3e3b3b
cf262fd
2e893d4
e5fff45
086429d
a19f085
3d0353d
fecec15
a478bfd
22d50c0
d7cced3
39b7c69
bf2d1b9
00d3cfa
9b1f648
7ef7f50
895f0cb
84e93e6
1764e17
79e3b64
85438af
6a82023
23f53b6
11fd40d
90c3e5e
33f5e1c
52b2fa9
a48c9d2
552e28c
47186df
36841ec
552ef61
ea4beec
61a5b8a
41e7dd5
fdad104
38fce6e
e9aa622
560c888
1518d19
ba09d04
2ec2e62
3da962d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do? Convert through c_str(), and resive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to reduce the size because the postgresl timestamp column does not accept the trailing
Z
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Another thing is that
CTime::currentDateTime()
already returns a CT::string, why go throughc_str()
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you check if the times in the table are correct if the DB and client are not running in UTC time zone?
Because I think inserting a time without timeonze into time
TIMESTAMPTZ
column might trigger timezone conversion.See here:
For timestamp with time zone, the internally stored value is always in UTC (Universal Coordinated Time, traditionally known as Greenwich Mean Time, GMT). An input value that has an explicit time zone specified is converted to UTC using the appropriate offset for that time zone. If no time zone is stated in the input string, then it is assumed to be in the time zone indicated by the system's [TimeZone](https://www.postgresql.org/docs/current/runtime-config-client.html#GUC-TIMEZONE) parameter, and is converted to UTC using the offset for the timezone zone.
Can we get the timestamp in a format like this to avoid the issue:
1999-01-08 04:05:06 -8:00
Maybe instead of resizing, just replace
Z
by0:00
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not able to insert timestamps with the suggested format. But I changed the type from TIMESTAMPTZ to TIMESTAMP and now I see UTC timestamps in the table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a cache, right? The idea is to avoid multiple queries to the DB when this function gets called for different layers and
metadataKey
, right?What if this function get called again with a different
datasetName
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is cache, in this case for this dataset.
There are currently no cases for multiple datasets in one process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is true (as all the tests works...), but at the very least the function comments should make this very clear. Or be safe and implement the case by storing the values in a map by
datasetName
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will be done in the follow up MR where we want to return all datasets with their layer info using one request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we haven't so far worried about SQL injection, but this one seems a bit easy to access:
Do a GetCapabilties call with a
DROP TABLES
some where in the dataset name would do the trick, I think?Probably for the already existing DB tables, the queries are by layer name, and these are checked in the dataset configs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the datasetname is checked early in the process.
Nevertheless I added the same check, just in case we forget or misuse it in another way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checkIfPathHasValidTokens()
is in other cases use to validate path, not sure it helps against SQL injection.I'll look at the other check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this change intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the original fillvalue was wrong, I saw this in the tests.