Add Dataset::has_capability

[Atreyagaurav]

• 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.

---

This is for doing things like Dataset::has_capability(DatasetCaps::ODsCTransactions) to check if dataset supports Transaction.

I added the Capabilities from:

• https://gdal.org/en/latest/doxygen/ogr__api_8h.html#a9010219bbc2e32627064ed860048d979
• https://gdal.org/en/latest/doxygen/gdal_8h.html#a8dc635a536f123e1b03004dc74b4da86

I followed the example of Layer::has_capability for both of them.

This will help use the if {} else {} block for capability based logic like the one mentioned in #582

Basically it'll make it so that @lnicola 's Suggestion given below can work:

let stuffs = |d: &Dataset| -> Result<()> { ... };
if /* transactions are supported */ {
    let txn = d.start_transaction()?;
    stuffs(&txn)?;
    txn.commit()?;
} else {
    stuffs(&d)?;
}

Currently, without this function, we have to wait for the result of Dataset::start_transaction to know if it supports it, in which case we cannot use if () {} else {} block due to Rust's borrow rules as the Dataset::start_transaction will take &mut Dataset.

[Atreyagaurav]

About the Enum variants, since clippy warns about the Prefix, should we just remove them? Since these are rust enum, I think we don't need the prefixes like ODrC, ODsC, OLC etc. on the enum itself as the Enum names (LayerCaps, DriverCaps, DatasetCaps etc.) are enough to distinguish them unlike in C.

I just followed what LayerCaps was doing for the other two. Changing that would be breaking change, but at least for the new ones we could not add Prefixes.

[rouault]

OGR_Dr_ is a legacy API.
The equivalent of ODrCCreateDataSource is testing the presence of GDAL_DCAP_CREATE in the driver metadata. Cf https://gdal.org/en/latest/doxygen/ogr__api_8h.html#a9010219bbc2e32627064ed860048d979
It looks we don't have an equivalent of ODrCDeleteDataSource though

[Atreyagaurav]

Maybe I should remove Driver::has_capability in that case. And we can add the new one later. Or accept the legacy API for now. But considering there are only two capability to check, I think it is fine to not have this method for now.

What do you think?

EDIT:
When I did dir(gdal) on python, I get these, if DCAP_CREATE is checked for create capability, then it makes sense to use others for similar capabilities as well. It is probably already implemented for Metadata check somewhere, though (but I guess we can make a method specific to check DCAP_ metadata).

['DCAP_COORDINATE_EPOCH', 'DCAP_CREATE', 'DCAP_CREATECOPY', 
'DCAP_CREATECOPY_MULTIDIMENSIONAL', 'DCAP_CREATE_FIELD', 'DCAP_CREATE_LAYER', 
'DCAP_CREATE_MULTIDIMENSIONAL', 'DCAP_CURVE_GEOMETRIES', 'DCAP_DEFAULT_FIELDS', 
'DCAP_DELETE_FIELD', 'DCAP_DELETE_LAYER', 'DCAP_FEATURE_STYLES', 'DCAP_FEATURE_STYLES_READ', 
'DCAP_FEATURE_STYLES_WRITE', 'DCAP_FIELD_DOMAINS', 'DCAP_FLUSHCACHE_CONSISTENT_STATE', 
'DCAP_GNM', 'DCAP_MEASURED_GEOMETRIES', 'DCAP_MULTIDIM_RASTER', 
'DCAP_MULTIPLE_VECTOR_LAYERS', 'DCAP_NONSPATIAL', 'DCAP_NOTNULL_FIELDS', 
'DCAP_NOTNULL_GEOMFIELDS', 'DCAP_OPEN', 'DCAP_RASTER', 'DCAP_RELATIONSHIPS', 
'DCAP_RENAME_LAYERS', 'DCAP_REORDER_FIELDS', 'DCAP_SUBCREATECOPY', 'DCAP_UNIQUE_FIELDS', 
'DCAP_VECTOR', 'DCAP_VIRTUALIO', 'DCAP_Z_GEOMETRIES']

[Atreyagaurav]

I'm waiting on whether to remove Driver::has_capability since it is based on legacy API, or rewrite it to use the DCAP metadata instead. Or maybe just adding a metadata checking function (if it doesn't exist already) is sufficient.

[lnicola]

I think we can do something like below, what do you say?

struct DatasetCapability(&'static CStr); // we could also support non-'static ones, but probably not worth it

impl DatasetCapability {
    pub const CREATE_LAYER: DatasetCapability = DatasetCapability(c"CreateLayer");
    pub const DELETE_LAYER: DatasetCapability = DatasetCapability(c"DeleteLayer");
}

struct Dataset;
impl Dataset {
    pub fn test_capability(&self, cap: DatasetCapability) -> bool {
        _ = cap.0;

        // this is the best dataset
        true
    }
}

This is a little shorter, while still allowing custom caps if the user ever needs one.

[Atreyagaurav]

@lnicola The DatasetCapability is not deprecated, and there are a fixed list on gdal side so I think it's fine.
https://gdal.org/en/stable/api/gdaldataset_cpp.html#_CPPv4N11GDALDataset14TestCapabilityEPKc

The Driver test capability is the one that is deprecated. So I think we can just remove it for now. I did initially made this to add a test condition to see if database supports transition. I added the Driver capability for completeness, but if it's deprecated it's fine.

Now this only contains Dataset::has_capability.

Edit:
It might be a good idea to make a function that tests using GDALGetMetadataItem(hDriver, GDAL_DCAP_CREATE) and similar, but I didn't find GDAL_DCAP_CREATE in gdal_sys, so I'm leaving it for later.

[Atreyagaurav]

Github again closed it while I was doing forced updates....

[Atreyagaurav]

Is that memory leak my fault? I'm using the same code as the one in Layer::has_capabilities that was already there.

[lnicola]

No, it's a bug in Rust 1.83, which was just released. It seems to be already fixed in beta.

[Atreyagaurav]

Thank you, can you merge with failed tests? or should I do something?

[lnicola]

The DatasetCapability is not deprecated, and there are a fixed list on gdal side so I think it's fine.

My version has much less code, runs slightly faster, and can still be extended if GDAL adds a new capability, so I still think it's an improvement.

Thank you, can you merge with failed tests? or should I do something?

I disabled the test, you can rebase.

[lnicola]

That wasn't a clean rebase or squash but.. whatever, I guess. Anyway, please capitalize datasource in the doc-comments, and add a . at the end, they're a bit hard to read right now.

Afterwards I think we can merge this and use it in #591?

[Atreyagaurav]

Sorry, I was struggling with rebase since the Valgrind was in the middle of two commits I made. It should be clean now, I undid everything and made a new commit on top.

[lnicola]

Yeah, it's clean now, thanks!

[Atreyagaurav]

Afterwards I think we can merge this and use it in #591?

About that, this one is so that we can manually do the "if supports transaction" part by ourselves.

I think the #591 is a bit different because even if starting transaction fails we want to continue writing to database as the transaction is optional and for speedup process only.

[lnicola]

If the driver supports transactions (like GPKG) and StartTransaction fails, I think it's fine to propagate that error to the user. It means something is really wrong and we can't do much about it.

[lnicola]

Well, I do hope that TRUE is 1 and not 0xFFFF_FFFF 😅.

[Atreyagaurav]

That's what test cases are for. It's working for now.