-
Notifications
You must be signed in to change notification settings - Fork 11
Kotlin: optional model arrays become List<T>? = null (breaking) #433
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
Open
jeremy
wants to merge
4
commits into
main
Choose a base branch
from
track-d-kotlin-optional-arrays
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
df162fe
Kotlin: emit every optional model array as List<T>? = null
jeremy 71d20d9
Rubric 1B.4: optional-field note covers all six SDKs + Go's encode-on…
jeremy 5761732
Address review: prove required arrays non-null, honest 1B.4, wire D g…
jeremy b2d031d
Address review round 2: UTF-8 reads, correct Go optional-scalar decod…
jeremy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,142 @@ | ||
| #!/usr/bin/env ruby | ||
| # frozen_string_literal: true | ||
|
|
||
| # D-invariant guard for the Kotlin generator's optional-array contract. | ||
| # | ||
| # Every OPTIONAL generated array must be `List<T>? = null` so an absent array | ||
| # stays distinct from a present-but-empty one (SPEC.md Optional Fields rule), | ||
| # aligning Kotlin with the other five SDKs. Every REQUIRED array stays a plain | ||
| # non-null `List<T>`. No generated array may default to the `= emptyList()` | ||
| # sentinel that this fix removed. | ||
| # | ||
| # Two levels of enforcement: | ||
| # | ||
| # * Schema-checked (generated/models/<Component>.kt whose basename is an | ||
| # OpenAPI component): each array property is cross-checked against the | ||
| # component's `required` set — a REQUIRED array MUST be non-null `List<T>` | ||
| # (no `?`, no default); an OPTIONAL array MUST be `List<T>? = null`. This | ||
| # proves both halves of the guarantee, so a required array mistakenly | ||
| # emitted as `List<T>?` is caught, not silently accepted. | ||
| # * Structural (everything else — request/param types in generated/services, | ||
| # supporting model types not present as components): a property with a | ||
| # default MUST be `List<T>? = null` (so `= emptyList()` or any non-null | ||
| # default fails). Requiredness can't be derived here, so a bare `List<T>` | ||
| # or nullable-without-default is left alone. | ||
| # | ||
| # Pins the D fix against regression. Wired into `make check`. | ||
|
|
||
| require "json" | ||
|
|
||
| PROJECT_ROOT = File.expand_path("..", __dir__) | ||
| GENERATED_DIR = File.join( | ||
| PROJECT_ROOT, | ||
| "kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated" | ||
| ) | ||
| MODELS_DIR = File.join(GENERATED_DIR, "models") | ||
| OPENAPI_FILE = File.join(PROJECT_ROOT, "openapi.json") | ||
|
|
||
| unless File.file?(OPENAPI_FILE) | ||
| warn "ERROR: openapi.json not found at #{OPENAPI_FILE}" | ||
| exit 2 | ||
| end | ||
|
|
||
| # Read as UTF-8 regardless of process locale (LC_ALL=C would otherwise read as | ||
| # US-ASCII and choke on the spec's / generated code's UTF-8 bytes). | ||
| components = JSON.parse(File.read(OPENAPI_FILE, encoding: "UTF-8")).dig("components", "schemas") || {} | ||
|
|
||
| errors = [] | ||
|
|
||
| files = Dir.glob(File.join(GENERATED_DIR, "**", "*.kt")).sort | ||
| if files.empty? | ||
| warn "ERROR: no generated Kotlin files found under #{GENERATED_DIR}" | ||
| exit 2 | ||
| end | ||
|
|
||
| # Parse one property line into [wire_name, type_part, default_part] or nil when | ||
| # the line isn't an array-typed `val`. `wire_name` is the @SerialName value when | ||
| # present, else the Kotlin field name (the generator only adds @SerialName when | ||
| # the camelCase name differs from the wire name, so this recovers the wire name). | ||
| def parse_array_property(line) | ||
| return nil unless line =~ /:\s*(List<.*)$/ | ||
|
|
||
| decl = Regexp.last_match(1).strip.sub(/[,)]\s*$/, "").strip | ||
| field = line[/\bval\s+(\w+)\s*:/, 1] | ||
| return nil unless field | ||
|
|
||
| serial = line[/@SerialName\("([^"]+)"\)/, 1] | ||
| wire = serial || field | ||
|
|
||
| type_part, default_part = | ||
| if decl.include?("=") | ||
| left, right = decl.split("=", 2) | ||
| [left.strip, right.strip] | ||
| else | ||
| [decl, nil] | ||
| end | ||
|
|
||
| [wire, type_part, default_part] | ||
| end | ||
|
|
||
| files.each do |path| | ||
| rel = path.sub("#{PROJECT_ROOT}/", "") | ||
| component_name = | ||
| if path.start_with?("#{MODELS_DIR}/") | ||
| base = File.basename(path, ".kt") | ||
| base if components.key?(base) | ||
| end | ||
| component = component_name ? components[component_name] : nil | ||
| required_fields = component ? (component["required"] || []) : nil | ||
| known_props = component ? (component["properties"] || {}) : nil | ||
|
|
||
| File.foreach(path, encoding: "UTF-8").with_index(1) do |line, lineno| | ||
| parsed = parse_array_property(line) | ||
| next unless parsed | ||
|
|
||
| wire, type_part, default_part = parsed | ||
| nullable = type_part.end_with?("?") | ||
| has_default = !default_part.nil? | ||
|
|
||
| # Schema-checked path: the field is a known property of an OpenAPI component. | ||
| if required_fields && known_props&.key?(wire) | ||
| if required_fields.include?(wire) | ||
| if nullable || has_default | ||
| errors << "#{rel}:#{lineno}: `#{wire}` is REQUIRED in schema " \ | ||
| "`#{component_name}` but is emitted as `#{type_part}" \ | ||
| "#{has_default ? " = #{default_part}" : ''}` — a required " \ | ||
| "array must be non-null `List<T>` with no default" | ||
| end | ||
| elsif !nullable || default_part != "null" | ||
| errors << "#{rel}:#{lineno}: `#{wire}` is OPTIONAL in schema " \ | ||
| "`#{component_name}` but is emitted as `#{type_part}" \ | ||
| "#{has_default ? " = #{default_part}" : ''}` — an optional " \ | ||
| "array must be `List<T>? = null`" | ||
| end | ||
| next | ||
| end | ||
|
|
||
| # Structural path: requiredness unknown (request/param types, supporting | ||
| # non-component models). Only a defaulted array is constrained. | ||
| next unless has_default | ||
|
|
||
| if default_part == "emptyList()" | ||
| errors << "#{rel}:#{lineno}: optional array uses the forbidden " \ | ||
| "`= emptyList()` sentinel — must be `List<T>? = null`" | ||
| elsif !nullable | ||
| errors << "#{rel}:#{lineno}: non-null array `#{type_part}` carries a " \ | ||
| "default (`= #{default_part}`) — an optional array must be " \ | ||
| "nullable (`List<T>? = null`)" | ||
| elsif default_part != "null" | ||
| errors << "#{rel}:#{lineno}: nullable array defaults to " \ | ||
| "`#{default_part}` — must default to `null`" | ||
| end | ||
| end | ||
| end | ||
|
|
||
| if errors.empty? | ||
| puts "==> Kotlin optional-array invariant clean — #{files.size} generated files scanned" | ||
| exit 0 | ||
| else | ||
| warn "Kotlin optional-array invariant failed:" | ||
| errors.each { |e| warn " - #{e}" } | ||
| exit 1 | ||
| end | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.