open-mpic / open-mpic-specification

This is the Open API specification for the Open Multi-Perspective Issuance Corroboration (open-mpic) project.
MIT License
4 stars 1 forks source link

Casing #4

Closed sciros closed 1 month ago

sciros commented 2 months ago

Right now all keys in the API are using kebab case ("word-word-word"). I think life for all implementers of the API (including me) would be much easier if if the keys were in snake case ("word_word_word").

The general reasoning for this is that automatic Json serialization/deserialization is FAR easier when you don't have to do case transformation, especially when a language's (hi, Python!) ecosystem is rather ill-suited for it. For some languages, camelCase is easier to handle (Java, C#) but those languages also have mature enough ecosystems around this that they can manage case transformation as needed through libraries that one can rely on sufficiently.

The big problem with kebab case is that there is virtually no language (that I've used anyway) which will accept it for auto-deserialization since object fields can't have hyphens in their names. So it's really between snake_case and camelCase, and I think with our known target languages and their capabilities, snake_case will give us by far the widest level and greatest ease of adoption.

Thoughts on this? I'll propose this on the IETF draft as well if you agree, and otherwise we can discuss further.

(I'd started refactoring validation by thinking "how much more elegant would it be if I could just deserialize into a domain object, then I don't have to have any magic strings anywhere in the request processing part of the code" and immediately ran into Python's struggle with kebab-case and the need to roll my own recursive case transformation function as a workaround, which kills a lot of the elegance. Also it makes it really painful to then serialize back into kebab case.)

bwesterb commented 2 months ago

ACME seems to use camel case. I don't have strong opinions here, so we can move the IETF draft to snake if you like.

birgelee commented 2 months ago

I am fine with snake case particularly if there is auto deserialization happening. I agree that there is essentially no language with builtin support for kebab case. I had used kebab case as this is recommended practice for the naming of REST API endpoints ( see https://www.theserverside.com/video/Top-REST-API-URL-naming-convention-standards ).

sciros commented 2 months ago

@birgelee yep that's 100% the right practice for API URL paths. And those we'll keep kebab case. It'll just be the payload included with the request (and the body of the response) that will be in snake case.

birgelee commented 2 months ago

sounds good. I would also be fine with the RFC drafts we are generating using this convention.

sciros commented 2 months ago

Here's a sample request (CAA check) and response you get if you run the integration test as of right now (Aug 27) in the ds-validation-logic branch. It's fairly verbose, as I went with my usual habit of descriptive (long) names. Elements in this output are unordered btw. Not sure if domain_or_ip_target (formerly identifier ... I'm open to other names) should be where it is or if we should put it elsewhere in the request. coordination_parameters sounded off. We really are doing process orchestration, even if it's a loaded word in a containerization-oriented context. I haven't renamed the coordinator, though. Anyway I changed the casing to snake case as you can see :-P

Request:

{
    "orchestration_parameters": {
        "perspective_count": 3,
        "quorum_count": 2,
        "domain_or_ip_target": "example.com",
        "max_attempts": null,
        "perspectives": null
    },
    "caa_check_parameters": {
        "certificate_type": "tls-server",
        "caa_domains": [
            "mozilla.com"
        ]
    }
}

Response:

{
    "request_orchestration_parameters": {
        "perspective_count": 3,
        "quorum_count": 2,
        "domain_or_ip_target": "example.com",
        "max_attempts": null,
        "perspectives": null
    },
    "actual_orchestration_parameters": {
        "perspective_count": 3,
        "quorum_count": 2,
        "attempt_count": 1
    },
    "is_valid": true,
    "check_type": "caa",
    "perspectives": [
        {
            "perspective": "us-east-2",
            "check_passed": true,
            "errors": null,
            "timestamp_ns": 1724733737509479740,
            "check_type": "caa",
            "details": {
                "caa_record_present": false,
                "found_at": null,
                "response": null
            }
        },
        {
            "perspective": "eu-west-2",
            "check_passed": true,
            "errors": null,
            "timestamp_ns": 1724733737778548673,
            "check_type": "caa",
            "details": {
                "caa_record_present": false,
                "found_at": null,
                "response": null
            }
        },
        {
            "perspective": "ap-northeast-2",
            "check_passed": true,
            "errors": null,
            "timestamp_ns": 1724733737927818565,
            "check_type": "caa",
            "details": {
                "caa_record_present": false,
                "found_at": null,
                "response": null
            }
        }
    ],
    "caa_parameters": {
        "certificate_type": "tls-server",
        "caa_domains": [
            "mozilla.com"
        ]
    }
}
birgelee commented 2 months ago

I generally like the new naming and schema. It is much more verbose, but I feel it has a lot of clarity. I also think this is optimal from a compliance perspective, so I am OK going ahead with it.

sciros commented 2 months ago

I'm also thinking of moving domain_or_ip_target and the upcoming check_type to top-level parameters, outside of orchestration_parameters (that's just the new name for system-params).

birgelee commented 2 months ago

sounds good, will this make orchestration_parameters optional?

sciros commented 2 months ago

Yes looks like it will! And we might still need to brainstorm about how to best accommodate the "diagnostics-only" params like perspectives. I actually have a TODO in the code about it, like does it just override the perspective_count since that's pretty much its purpose, so then the older logic of "can't specify both perspectives and perspective count" can be eliminated, etc.

birgelee commented 1 month ago

closing since latest version uses snake_case