aries-cloudagent-python icon indicating copy to clipboard operation
aries-cloudagent-python copied to clipboard

timestamp data type and format issue in AdminAPI

Open stevendonline opened this issue 4 years ago • 4 comments

In ACA-Py Administration API swagger.json, the time data format e.g timestamp or range etc have "int32" as "format" value, however the "maximum" value is 18446744073709551615 which needs long or int64 to support, this has caused problems with some api tools e.g NSwag when generating api client.

    "IndyRequestedCredsRequestedPred": {
        "properties": {
            "cred_id": {
                "description": "Wallet credential identifier (typically but not necessarily a UUID)",
                "example": "3fa85f64-5717-4562-b3fc-2c963f66afa6",
                "type": "string"
            },
            "timestamp": {
                "description": "Epoch timestamp of interest for non-revocation proof",
                "example": 1640995199,
                **"format": "int32",**
                **"maximum": 18446744073709551615,**
                "minimum": 0,
                "type": "integer"
            }
        },
        "required": ["cred_id"],
        "type": "object"
    },
	

BTW. Is there recommended tool for generating c# api client for adminapi?

stevendonline avatar Dec 08 '21 12:12 stevendonline

I've run into the same issue @stevendonline. See also my other issue here: https://github.com/hyperledger/aries-cloudagent-python/issues/1485

We currently maintain a patch file that we regenerate for every release of ACA-Py where we fix issues with the generated openapi: https://github.com/didx-xyz/aries-cloudcontroller-python/blob/main/generator/data/openapi.patch with the output file here: https://github.com/didx-xyz/aries-cloudcontroller-python/blob/main/generator/data/openapi.yml

This also contains an operation id map so the generated methods names are more human friendly: https://github.com/didx-xyz/aries-cloudcontroller-python/blob/main/generator/data/operation-id-map.yml. They are however idiomatic for python, so would probably need to tweak that for .NET.

This is for verision 0.7.1, version 0.7.2 has a pr open here: https://github.com/didx-xyz/aries-cloudcontroller-python/pull/77

Indicio also maintains an aca-py openapi repo, which is quite similar to the links posted above (we keep 'borrowing' code from each other 😁) but doesn't contain the int32 fixes yet. I hope to merge our changes back into there sometime.

As per generating a .NET client, we build our own on top of openapi-generator because all python ones did not meet our requirements. It has been working really great I must say. The only downside is the optionality of some fields in the swagger which are not optional, giving type errors. For .NET (and other languages) I've heard good things about autorest

Hope this helps!

TimoGlastra avatar Dec 08 '21 12:12 TimoGlastra

Thank you @TimoGlastra for the prompt response. Answers like this really help!

stevendonline avatar Dec 08 '21 13:12 stevendonline

Thanks @TimoGlastra . Some questions from that explanation:

  • Do the patches get pushed back into ACA-Py? Do they need to?
  • Should the maximum be removed or changed?
  • Would it be useful the tools that you and Indicio run to be internal to ACA-Py? Can the community benefit from that?
  • There is a document in the repo - UsingOpenAPI.md that has some guidance for developers. Is there anything from this explanation that could/should be PR'd into that document?

Thanks

swcurran avatar Dec 08 '21 14:12 swcurran

Do the patches get pushed back into ACA-Py? Do they need to?

I've done a few minor updates over the past months, but not all of them are merged back. I think we should do it, just haven't gotten around to it. Mainly because I would also like to tackle the optionality issue then (a lot of response type properties are marked as optional even though they are always present, making the generated object less useful) which would take more time.

Should the maximum be removed or changed?

Yes! I think however there is a limitation in the swagger library ACA-Py uses, so we would probably need to remove the check completely, but also not sure if that will fix it.

Would it be useful the tools that you and Indicio run to be internal to ACA-Py? Can the community benefit from that?

I would say yes and yes! Some are really use case specific, but it would mostly be an extension to the current openapi generator that is already present in ACA-Py and do some cleanups. However if we manage to fix most of them, maybe it won't be needed

There is a document in the repo - UsingOpenAPI.md that has some guidance for developers. Is there anything from this explanation that could/should be PR'd into that document?

I think a pointer to the processing scripts is definitely useful to have there

TimoGlastra avatar Dec 08 '21 17:12 TimoGlastra