Skip to content
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

Breaking Change: Standardize casing for property names and enumerated values in JSON #642

Open
michaeltlombardi opened this issue Feb 13, 2025 · 7 comments · May be fixed by #645 or #648
Open

Breaking Change: Standardize casing for property names and enumerated values in JSON #642

michaeltlombardi opened this issue Feb 13, 2025 · 7 comments · May be fixed by #645 or #648
Assignees
Labels
Issue-Enhancement The issue is a feature or idea Needs Triage
Milestone

Comments

@michaeltlombardi
Copy link
Collaborator

Summary of the new feature / enhancement

As a user of DSC, resource author, and contributing or integrating developer, I want to rely on consistent casing for JSON that DSC emits and expects, instead of having to double-check properties and values or accidentally run into unexpected validation or (de)serialization errors.

Currently, the general practice is to use camelCase for JSON property names and enumerated values. However, not all of the structs follow this convention, and it's not predictable which values or properties use a different casing.

Because JSON Schemas are case sensitive, this constitutes a breaking change best made sooner than later.

Proposed technical implementation details (optional)

  1. Ensure that the serde attribute for rust structs and enums correctly cases the property names and values.
  2. Update the JSON schemas hosted in the repository to match.
  3. Ensure all future structs and enums that derive the Serialize and/or Deserialize use camel casing.
@michaeltlombardi michaeltlombardi added Issue-Enhancement The issue is a feature or idea Needs Triage labels Feb 13, 2025
@SteveL-MSFT SteveL-MSFT added this to the 3.0-Approved milestone Feb 18, 2025
@SteveL-MSFT SteveL-MSFT self-assigned this Feb 18, 2025
@SteveL-MSFT
Copy link
Member

@michaeltlombardi I manually reviewed the structs that are (de)serialized. The only cases I saw were in Progress which is already fixed by #644 and tracing supporting line_number which is what the tracing crate uses, but created PR to allow for lineNumber alias. Is there some other instance(s) you're aware of that need to be fixed?

@michaeltlombardi
Copy link
Collaborator Author

I found the following definitions other than progress—I looked primarily for structs and enums that had both serialize/deserialize or the JsonSchema trait:

  • #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema)]
    pub enum ContextKind {
    Configuration,
    Resource,
    }
  • #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema)]
    pub enum SecurityContextKind {
    Current,
    Elevated,
    Restricted,
    }
  • #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema)]
    pub enum Operation {
    Get,
    Set,
    Test,
    Export,
    }
  • #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema)]
    pub enum ExecutionKind {
    Actual,
    WhatIf,
    }
  • #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema)]
    pub enum ConfigurationResourceCompletionStatus {
    Success,
    Failure,
    }
  • #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema)]
    pub enum MessageLevel {
    Error,
    Warning,
    Information,
    }
  • #[derive(Clone, Debug, PartialEq, Deserialize, Serialize, JsonSchema)]
    pub enum Capability {
    /// The resource supports retrieving configuration.
    Get,
    /// The resource supports applying configuration.
    Set,
    /// The resource supports the `_exist` property directly.
    SetHandlesExist,
    /// The resource supports simulating configuration directly.
    WhatIf,
    /// The resource supports validating configuration.
    Test,
    /// The resource supports deleting configuration.
    Delete,
    /// The resource supports exporting configuration.
    Export,
    /// The resource supports resolving imported configuration.
    Resolve,
    }
  • #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema)]
    pub enum Kind {
    Adapter,
    Group,
    Importer,
    Resource,
    }

I also noticed the progress struct, which only serializes - not sure if we should also have it with a JSON Schema if it's meant to be consumed by other tools, and thus also camelCased:

DSC/dsc_lib/src/util.rs

Lines 42 to 50 in 3963389

#[derive(Default, Debug, Clone, Serialize)]
pub struct Progress {
pub activity: String,
pub percent_complete: u16,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub status: Option<String>,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub seconds_remaining: Option<u64>,
}

@SteveL-MSFT
Copy link
Member

I think PascalCasing makes sense for Enums which are used as property values and not property names. Progress currently is only serialized, when we get to enabling resources to emit progress then deserialization will be important.

michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this issue Feb 18, 2025
Prior to this change, the structs and enums in `dsc_lib` didn't consistently
use camelCase - most property names and enum values use camelCase, but not
all - and this inconsistency isn't predictable for end users, who need to
consult the JSON Schemas to be sure.

This change updates the definitions to rename the fields and values when
serializing and deserializing, which also updates their JSON Schema.

A future change is required to update the canonical schemas in the
repository to match these updates.
@michaeltlombardi
Copy link
Collaborator Author

Pascal casing for enums is currently inconsistent, and some of them can't be changed to PascalCasing without breaking ARM compatibility, like DataType:

#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema)]
pub enum DataType {
#[serde(rename = "string")]
String,
#[serde(rename = "secureString")]
SecureString,
#[serde(rename = "int")]
Int,
#[serde(rename = "bool")]
Bool,
#[serde(rename = "object")]
Object,
#[serde(rename = "secureObject")]
SecureObject,
#[serde(rename = "array")]
Array,
}

Other enums that currently use camelCasing:

  • #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema)]
    pub enum InputKind {
    /// The input is accepted as environmental variables.
    #[serde(rename = "env")]
    Env,
    /// The input is accepted as a JSON object via STDIN.
    #[serde(rename = "stdin")]
    Stdin,
    }
  • #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema)]
    pub enum ReturnKind {
    /// The return JSON is the state of the resource.
    #[serde(rename = "state")]
    State,
    /// The return JSON is the state of the resource and the diff.
    #[serde(rename = "stateAndDiff")]
    StateAndDiff,
    }
  • #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema)]
    pub enum ConfigKind {
    /// The adapter accepts full unprocessed configuration.
    #[serde(rename = "full")]
    Full,
    /// The adapter accepts configuration as a sequence.
    #[serde(rename = "sequence")]
    Sequence,
    }

@SteveL-MSFT
Copy link
Member

ARM compat is a priority, so I guess that means we should make everything camelCased to be consistent. I'll update my PR for this.

@michaeltlombardi
Copy link
Collaborator Author

Relatedly, in my ideal world, which I am not recommending we do in the short-to-mid-term, or without further user feedback:

  1. DSC property names and enum values are case-insensitive
  2. Provide a way for resources to indicate that their properties/values are case-insensitive
  3. DSC is able to inform users about casing issues / canonicalize casing in the dsc config resolve command

Because DSC knows the supplied data and JSON Schema for not only its own data types but also for resource instances, I think we can (relatively cheaply, maintainably) implement a check for case-sensitivity values. The jsonschema crate supports custom keywords we could use for handling this, too.

@SteveL-MSFT
Copy link
Member

I think making property names and enum values case-insensitive makes sense since we don't ever want a situation where we have different properties/enums that differ by casing. It looks like serde has support for this so I'll play with it.

@SteveL-MSFT SteveL-MSFT linked a pull request Feb 18, 2025 that will close this issue
3 tasks
michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this issue Feb 18, 2025
Prior to this change, the structs and enums in `dsc_lib` didn't consistently
use camelCase - most property names and enum values use camelCase, but not
all - and this inconsistency isn't predictable for end users, who need to
consult the JSON Schemas to be sure.

This change updates the definitions to rename the fields and values when
serializing and deserializing, which also updates their JSON Schema.

A future change is required to update the canonical schemas in the
repository to match these updates.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Enhancement The issue is a feature or idea Needs Triage
Projects
None yet
2 participants