Mapping of additional methods for SpatialRef.

[tombieli]

• I agree to follow the project's code of conduct.
• I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

---

Hello,
I wrote additional methods for SpatialRef to access some properties from OGRSpatialReference.
I needed those properties in project which I'm prototyping :) .
Of course I've added also relevant tests.

Best Regards,
Tomasz Bieliński

[metasim]

@tombieli Thanks for the PR! Would you be willing to document the methods you added?

Here are some examples of what we're looking for:

https://github.com/georust/gdal/blob/master/src/vector/ops/transformations.rs

[tombieli]

@tombieli Thanks for the PR! Would you be willing to document the methods you added?

Here are some examples of what we're looking for:

https://github.com/georust/gdal/blob/master/src/vector/ops/transformations.rs

@metasim:
Yes. A commit is in the branch.
Best Regards

[metasim]

Would also like to get a review from one of the other maintainers.

[tombieli]

@metasim:
I've applied Your suggestions. I've also applied changes done by cargo fmt, I saw that some CI/CD pipelines crashed, because of not very well formatted code.

[tombieli]

Hello @lnicola,
I've applied changes suggested by You.
Best Regards,
Tomasz Bieliński

[lnicola]

Thanks, I'll take a look in a bit.

[lnicola]

To be honest, I'm not really a fan of these, since they basically say "the function returns Err if the C function returned an error", which is true, but useless.

But I don't mind them too much, so it's all right if you like them.

[tombieli]

I maybe tried to be to precise and I created to explicit documentation.

[lnicola]

Docs are good, but this is currently neither consistent with the rest of the documentation, nor useful: "the function returns Err if an error has occurred" doesn't bring any information at all to someone used to the Rust conventions.

Looking at GetSemiMajor, it's a bit weird -- it returns both a valid value (SRS_WGS84_SEMIMAJOR) and an error if it can't find the value, which is something I'm not sure we want to emulate.

Can you change this to something like "Returns an error if the semi-major axis can't be found" ("semi-minor" for the other one), "if there the PROJCS node is missing" for set_proj_param?

GetProjParm returns an error if it can't find PROJCS or CONVERSION, or if the parameter is missing. OSRSetAttrValue returns an error if the path is empty. OSRGetAttrValue returns an error if the attribute can't be found.

For geog_cs, it's a bit complicated, I guess an error means that it's missing.

These expose some of the GDAL internals, but I don't expect them to change significantly.

Also, OSRSetAttrValue in GDAL can take a null value, which means that the node is created without a value. Maybe we should take an Option<&str> to support that?

[tombieli]

@lnicola:
I've corrected comments.

I've also decided to change signature of get_proj_param to:

pub fn get_proj_param(&self, name: &str) -> Result<Option<f64>>

I did not include a default value option, because in each case this value will be discarded.

I've done similar thing to get_attr_value.

There is also problem with set_attr_value. I've tried to write some tests which set some silly (like "Spam" or "Eggs") and less silly (like "WGS_1984" and "metre") data, but results were quite strange. I'm not an expert at manipulation of WKT for projection, so I maybe tried to write tests in wrong way. Maybe some of You will have better idea how to write those tests.

Alternatively we can remove this function now and implement it later.

[lnicola]

but results were quite strange

You're right, I'll try to look into it.

[lnicola]

I took the liberty of pushing a commit to fix this and add a test. It was a pretty basic gotcha -- Option::map consumes the value. I would have hoped that clippy would catch this, but alas...

[tombieli]

Thanks. Rust/C++ edge can be tricky.

[lnicola]

Everything can be tricky. The Python bindings will segfault if you do something similar with a SpatialRef, so it's not just us.

[lnicola]

So ideally, this would return something like Result<Option<f64>> and the user could handle the missing case. But since we can't do that, should we take the default as an argument? Just noticed the other method, should we drop this one and rename?

It's currently ambiguous. Are the parameters never 0 in practice?

[tombieli]

Behavior of OSRGetProjParm is quite tricky, a least from Rust point of view.

In C and C++ if value is found it is returned normally, but if value is not found, a value from default parameter is returned, but also error code (if pointer is not null) is set to OGRERR_FAILURE. However looking from Rust point of view it may mean not found (therefore is candidate for Option::None) or can mean any other failure.
You can see this in GDAL source code for OSRGetProjParm.

To sum up:

• on existing value: we receive the value itself,
• if value does not exist: we receive default value and very general error code.

That's why I used only Result<f64> as return type and I also created also get_proj_param_or_default.

[lnicola]

Yeah, I'm inclined towards something like a single pub fn get_proj_param(&self, name: &str) -> Result<Option<f64>>, which returns Err on a string conversion error (basically never), and Ok(None) on OGRERR_FAILURE. It's not that awkward to use, and should be pretty clear.

Both error cases in the implementation are similar, and boil down to "there's no parameter with that name on the PROJCS".

EDIT: removed the default param.

[metasim]

Yeah, this is all unfortunate legacy API leakage. I don't know what we should do, or if there are other examples out there (e.g. proj?) but what if we had a new error type defined that indicates NotSet? Then, you clould go back to returning Result<f64> and users who care about a default can match on Err(NotSet) to handle it?

[lnicola]

Added one more commit with an assert! in get_attr_value.

r? @jdroenner, otherwise I'll r+ tomorrow or so.

[tombieli]

Added one more commit with an assert! in get_attr_value.

@lnicola:
I have a question: is this a good idea? As far as I know assert! will introduce potential panic! to production code. Maybe better solution will to return some Err or discard check entirely? C++ code should handle this properly.

[lnicola]

@tombieli first of all, my bad, I shouldn't have pushed this commit, but rather left it as a review comment. I only did it because I felt bad for letting this PR linger for so long, and wanted to get the new release out of the gate soon. And I actually mentioned it in the same comment where I suggested using usize. I would revert it now, but it feels like extra churn.

That said, to address your points:

C++ code should handle this properly.

The C++ code doesn't handle this properly, because the issue is in the Rust wrapper. C++ takes i32 (on common platforms), we have an usize (basically u64). Without the assert!, a large value like 0x8000_0000_0000_0000 will end up after conversion as 0. GDAL won't be able to tell because it will happen in our own code.

or discard check entirely

That feels like sweeping the problem -- which we introduced, by taking usize to make for a nicer API -- under the rug, allowing the caller to make unwanted changes to the SpatialRef, like in my example above.

So the question is whether to panic or return an error. My reasoning is that the caller must have gotten the index from somewhere. As far as I can tell, there are three ways one could get it:

• the caller knows the structure of the SpatialRef, and changes one specific node (like we do in our test); in this case, passing something like 0x8000_0000_0000_0000 is clearly incorrect (since GDAL can't even represent that), and we should panic since it's a caller bug
• the caller got the index from FindProjParm: in that case, it can't be outside of int (since GDAL returns one), so if the index is outside of the int limits, it's again a caller bug. We don't currently expose that function, but this is true regardless of that.
• the caller just used SetNode (OSRSetAttrValue) to create a new child, so they know that 0 is a valid index. If I'm reading the docs correct, that will create at most one child. And even if there was an AddNode method (which I couldn't spot), you wouldn't be able to create more than c_int::MAX children, since GDAL clearly can't represent that.

So I don't see any way code that's not hopelessly broken could end up with such a large index there. And I think in Rust it's rather common to panic in such cases, instead of allowing the program to keep running, only to crash or fail in a more obscure way later on.

But if we do decide to keep the panic, we should document it just in case.

[tombieli]

@lnicola: Thank You for Your answer.
First of all: I'm not upset that You made this commit - You are maintainer here.

I questioned this move with assert!, because I know there are two ideas in Rust programming:

• To represent all good and bad states of application as types. For instance the Result type and other enums,
• Not to use panic! in production code. I've heard that is even macro or some other CI/CD pipeline plugin that fails entire build, when linking to panic! is detected.

But looking from other side: I see on this simple example, that mapping API to Rust from C++ could be very tricky, especially when C++ library has its own non-rust ideas to create function contract (for instance: using signed integer for non-negative values and three-state result/error reporting).

I understand that chances to fire that assert! is quite low.

I don't have big experience in Rust, and no experience in shipping production code in Rust. I know that having theoretical bright ideas is not enough, so I don't expect that my opinion on this topic is 100% accurate (it may even less than 50% ;) ). If You as maintainer want this assert! I accept it.

But if we do decide to keep the panic, we should document it just in case.

I will add relevant info in docs.

[lnicola]

I'm not upset that You made this commit - You are maintainer here.

I'm not the only maintainer, and even if I were, it wasn't a nice thing to do.

Not to use panic! in production code. I've heard that is even macro or some other CI/CD pipeline plugin that fails entire build, when linking to panic! is detected.

I understand that viewpoint, but i think even the standard library will panic at times, for example when it detects that an invariant of a data structure got broken. RefCell does it if used incorrectly. Array indexing panics, but that's a trivial case. Memory allocators generally abort() when they detect corruption. So do other security mechanisms like stack cookies. In general, never panicking might be nice, but it's hard to achieve, and it often has its costs.

One pattern I've seen time and time again (especially in other languages) was:

• someone thinks exceptions are a bad thing because they can lead to crashes and error messages
• they write a function that might fail, catch and swallow any exceptions and return a null
• that function is called by another function, which "handles" the null the best way it can -- returning null on its own, since crashing would be bad
• 15 functions and 30 minutes later, the null ends up in a class field, something tries to use it and gets a NullPointerException
• the maintainer of the code fixes this by adding one more null check
• next time, it takes even longer to crash, and it's impossible to figure out where the original error happened

So I'm firmly in the "crash early if the caller is hopelessly buggy" camp, since limping on with some corrupted state will be much harder to detect. And I've tried to show earlier that there's no way a caller could pass a value larger than c_int::MAX without having done something very, very wrong.

---

But as I said, I'm not the only maintainer. It might be interesting to see other opinions on this.

[jdroenner]

I think the solution with the assert! is fine. I guess we can't avoid it in some cases...

[lnicola]

Can you rebase (or merge) to fix the conflict?

[tombieli]

Can you rebase (or merge) to fix the conflict?

Yes. Merge commit is in the branch.

[lnicola]

bors r=metasim,lnicola

[bors]

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• ci gdal-32
• ci gdal-32 (--all-features)
• ci gdal-33
• ci gdal-33 (--all-features)
• ci gdal-34
• ci gdal-34 (--all-features)
• ci gdal-35
• ci gdal-35 (--all-features)
• ci ubuntu-lts
• ci ubuntu-lts (--all-features)