Adds ISL model #251
Adds ISL model #251
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #251 +/- ##
============================================
- Coverage 85.47% 83.56% -1.91%
- Complexity 686 704 +18
============================================
Files 78 93 +15
Lines 2340 2531 +191
Branches 463 509 +46
============================================
+ Hits 2000 2115 +115
- Misses 225 299 +74
- Partials 115 117 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
- Marking classes as
dataclasses means that we can't hide implementation details as well.dataclasses always indirectly expose the primary constructor through the auto-generatedcopy()implementation, anddataclasses always have destructuringcomponentN()functions. I would like to eventually make all of the constructorsinternaland provide a version-safe builder for constructing the ISL model. The benefit ofdatais that we also get autogeneratedequals()andhashCode()functions. Should we not use data classes so as not to leak implementation through thecopy, etc. functions?
Sorry I am not that much familiar with Kotlin standard way for implementing a model but I think we should not expose any implementation detail. You mentioned making constructors internal for version-safe builders, if that showcases the right implementation detail based on version or if that hides the implementation detail then we should use that.
- Right now the constraint classes are nested in the
Constraintinterface. Thoughts? Should they be moved to a...ionschema.model.constraintspackages?
I think it should be okay to have them in the Constraint interface since we are just defining the model here which doesn't do any validation.
- I plan to have an
internal sealed interface SealedConstraint(once we upgrade the language level). Is it worth makingSealedConstraintspublic? If it is public, it would have to be gated by an opt-in annotation because every time we add a new constraint, it would be a breaking change. (See "TODO" comment inConstraint.ktfor more discussion.)
Did you mean we can provide an opt-in for whether to consider it as breaking change or not? I think it depends on which ISL version is used by the user to model schema. If they are using an older version they don't need the build failures. So if we are trying to provide something like that with the opt-in then we can make it public.
| package com.amazon.ionschema.model | ||
|
|
||
| /** | ||
| * A TypeArgument represents (defines or references) a Type, allowing the Type to be used as an argument for a constraint. |
There was a problem hiding this comment.
Should we add a grammar explaining what are these TypeArguments or maybe add a link to type reference section of the specification?
There was a problem hiding this comment.
I will, once the link exists.
| * Convenience alias for open content in schema headers, footers, and type definitions. | ||
| * Called 'OtherFields' to make it clear that this is for struct fields. | ||
| */ | ||
| typealias OtherFields = List<Pair<String, IonValue>> |
There was a problem hiding this comment.
(Question) Is this referring to open contents as per ISL 1.0? Maybe we should specify what ISL version will use this and maybe an example of what OtherFields would look like for a schema?
There was a problem hiding this comment.
This is independent of the schema version. It's just a container to hold extra struct fields.
| * See relevant section in [ISL 1.0 spec](https://amazon-ion.github.io/ion-schema/docs/isl-1-0/spec#fields) and | ||
| * [ISL 2.0 spec](https://amazon-ion.github.io/ion-schema/docs/isl-2-0/spec#fields). | ||
| */ | ||
| data class Fields internal constructor(val types: Map<String, VariablyOccurringTypeArgument>, val closed: Boolean) : SealedConstraint |
There was a problem hiding this comment.
(Suggestion) Since we already have constraint construction methods such as AnnotationsV* maybe we should provide different methods where fields,element as well? This would be less confusing and would be similar to how we handle other constraints like Annotations. Alternatively, we could also provide something like DistinctFields where it will be used for distinct fields and for non distinct fields Fields can be used.
There was a problem hiding this comment.
Alternatively we could either model v1 in it's own interface or not at all?
There was a problem hiding this comment.
I'm trying to have only one model that is used for all versions, and I will create version-specific builders so that they can only be constructed in valid ways.
The reason there are two "annotations" constraints is because the old annotations has some weird behavior (see #150). The differences between the two are so great, and it didn't seem worth it to try to figure out all of the weird edge cases, so I've just tried to avoid doing anything with the old annotations implementation so that I don't break anything.
None of the other ISL 1.0 constraints are as bad as that, so I haven't given up on them in the same way.
If you feel particularly strongly about this, I can try to emulate the ISL 1.0 annotations using the ISL 2.0 annotations constraint in order to unify the model.
|
|
||
| /** | ||
| * This internal only interface allows us to use an exhaustive `when` within this library. | ||
| * It will become `sealed` when we update to kotlin language version = 1.5 or higher. |
There was a problem hiding this comment.
1.5 was released almost two years ago, which is a blink in java time but pretty significant for Kotlin. What is keeping us on 1.4?
There was a problem hiding this comment.
Mostly just because we haven't determined whether an upgrade would require a new major version, and we haven't had a particularly strong impetus before now.
| * TODO—some library users might prefer for their builds to break when they are not handling a new constraint. Should | ||
| * we make this public with the caveat that it requires opt in, and is subject to breaking changes? |
There was a problem hiding this comment.
In looking at what you have here my sense is that not many if at all.. and that making it sealed would prevent folks from implementing their own... which I think we want to allow. So I would not make it sealed.
There was a problem hiding this comment.
I think we do not want to allow people to implement their own (for now). There was discussion about this as part of the ISL 2.0 spec work. We declined to introduce custom constraints (for now) because there was no concrete use case with which to justify the cost in complexity.
However, one thing we did decide is that if we do add user-defined constraints, we need to have support for it in the Ion Schema Specification, or else we'll start running into problems with schemas not being portable (i.e. different behavior for the same inputs on differently configured systems). For example, a custom constraint using the "is_prime" field name—if Ion Schema Kotlin on one system has that constraint installed, and on another system, Ion Schema Kotlin does not have that constraint installed, then the same schema and the same data could have different validation results on two different machines.
| * See relevant section in [ISL 1.0 spec](https://amazon-ion.github.io/ion-schema/docs/isl-1-0/spec#annotations). | ||
| */ | ||
| @Deprecated("The annotations constraint in Ion Schema 1.0 has poorly defined behavior.") | ||
| data class AnnotationsV1 internal constructor(val annotations: List<Annotation>, val closed: Boolean, val ordered: Boolean) : SealedConstraint { |
There was a problem hiding this comment.
Why is it valuable to model the deprecated constraint here?
There was a problem hiding this comment.
All Ion Schema 2.0 implementations are required to be able to import/operate on ISL 1.0. I probably should not actually mark it as @Deprecated.
| * See relevant section in [ISL 1.0 spec](https://amazon-ion.github.io/ion-schema/docs/isl-1-0/spec#fields) and | ||
| * [ISL 2.0 spec](https://amazon-ion.github.io/ion-schema/docs/isl-2-0/spec#fields). | ||
| */ | ||
| data class Fields internal constructor(val types: Map<String, VariablyOccurringTypeArgument>, val closed: Boolean) : SealedConstraint |
There was a problem hiding this comment.
Alternatively we could either model v1 in it's own interface or not at all?
| * A `DiscreteRange` is allowed to be a _degenerate interval_ (i.e. `start == endInclusive`), but it may not be an | ||
| * empty interval (i.e. `start > endInclusive`). | ||
| */ | ||
| data class DiscreteRange<T : Comparable<T>>(val start: T?, val endInclusive: T?) { |
There was a problem hiding this comment.
Why model this as it's own class? It seems like it would be simpler to just merge it with the above.
There was a problem hiding this comment.
Discrete Ranges don't require us to store whether the boundary value is exclusive because we can "translate" exclusive to +/-1 of the boundary value.
More importantly, however, is the fact that the logic for determining emptiness is different. With a discrete range, it is true that ∀n∈Z, ¬∃x∈Z, n<x & x<n+1, but for a range over continuous values, it would not be empty. If n and x are both in the set of all rational numbers rather than the set of all integers, then we can trivially prove that there does exist at least one x, such as x = n + 0.5.
Finally, my admittedly subjective opinion is that it helps the reader/user understand how the ranges are to be used by keeping them distinct.
There was a problem hiding this comment.
But whether a range is continuous or discrete depends on the type of arguments, right? I don't see any constraints that model that... so it looks like I can define a Discrete range of a Continuous (but Comparable) type and vice versa.
Further I don't see any definition in the spec about emptiness or degenerate-ness so I don't see where the requirement for the validation (and the need to split it) even comes from.
What I do see is what appears to be duplicated code. 🤷
There was a problem hiding this comment.
The test suite requires that ranges (at least for countable things like number of codepoints) are non-empty, and I believe that requirement has been part of the reference implementation since the initial release. https://github.com/popematt/ion-schema-tests/blob/master/ion_schema_2_0/constraints/codepoint_length.isl#L79
At this point, the non-empty range requirement is de facto part of the specification, and the specification document should be updated to reflect that.
Degenerate ranges are fine. I could remove that bit of the sentence if it's too confusing to have it, though.
There was a problem hiding this comment.
I have found a clean way to avoid the duplicated code, though, so I will remove the DiscreteRange<T> implementation.
| */ | ||
| None, | ||
| /** | ||
| * Ion Schema 1.0 nullability. See https://amazon-ion.github.io/ion-schema/docs/isl-1-0/spec#core-types |
There was a problem hiding this comment.
I'm pretty inclined to make modeling ISL 1 a non-goal.
There was a problem hiding this comment.
I don't think it's an option because of the requirement for ISL 2.0 implementations to also support ISL 1.0
| val type: List<String> = emptyList(), | ||
| val header: List<String> = emptyList(), | ||
| val footer: List<String> = emptyList(), |
There was a problem hiding this comment.
hrrm. The problem is that Kotlin's List is read-only but not technically immutable. IIRC, like internal this is something that can be circumvented with java code. Not sure how protective we want to get, just making sure you're aware of it.
There was a problem hiding this comment.
I am aware. If we're concerned about this, I could bring in the kotlinx.collections.immutable library.
ion-schema/src/test/kotlin/com/amazon/ionschema/model/ContinuousRangeTest.kt
Outdated
Show resolved
Hide resolved
I agree. I don't think there is anything to hide. |
ion-schema/src/test/kotlin/com/amazon/ionschema/model/SchemaDocumentTest.kt
Show resolved
Hide resolved
| * A `DiscreteRange` is allowed to be a _degenerate interval_ (i.e. `start == endInclusive`), but it may not be an | ||
| * empty interval (i.e. `start > endInclusive`). | ||
| */ | ||
| class DiscreteIntRange private constructor(private val delegate: ContinuousRange<Int>) { |
There was a problem hiding this comment.
Yea, I was sort of thinking that the only type we really needed this for was Int...
Rename file to match class?
There was a problem hiding this comment.
Is sort of a shame we need to delegate here... I get why Kotlin disallows extension of data classes. Personally I would consider a Range sealed base class that each specialization extended. 🤷
There was a problem hiding this comment.
Rename file to match class?
Good catch!
| interface Bounded<T> { val value: T } | ||
| data class Closed<T : Comparable<T>>(override val value: T) : Limit<T>(), Bounded<T> | ||
| data class Open<T : Comparable<T>>(override val value: T) : Limit<T>(), Bounded<T> | ||
| class Unbounded<T : Comparable<T>> : Limit<T>() { |
There was a problem hiding this comment.
Could this just be object and avoid the overriding of equals and hashcode? or does that not play well with java?
There was a problem hiding this comment.
Actually, it doesn't play well with generics. object Unbounded: Limit<Nothing>() was giving me trouble getting the variances right, but you are right that objects are not as nice in Java.
Issue #, if available:
#250
Also, partial work on #109 in that this PR has new range implementations that will eventually replace the existing range implementations.
Description of changes:
Adds a model for Ion Schema. Currently the model is opt-in only—gated by the
@ExperimentalIonSchemaModelannotation—and subject to breaking changes.A few questions/comments for reviewers:
element,all_of, etc.). This is a departure from the spec, but I believe it will eliminate the confusion between a "reference" and a so-called "type reference" (that could be a "reference", an inline import, or an inline type). I plan to update the spec to use the new terminology.regex.ktutility file so that the regex validation code can be shared (for now) between the model and the existing constraint impl.dataclasses means that we can't hide implementation details as well.dataclasses always indirectly expose the primary constructor through the auto-generatedcopy()implementation, anddataclasses always have destructuringcomponentN()functions. I would like to eventually make all of the constructorsinternaland provide a version-safe builder for constructing the ISL model. The benefit ofdatais that we also get autogeneratedequals()andhashCode()functions. Should we not use data classes so as not to leak implementation through thecopy, etc. functions?Constraintinterface. Thoughts? Should they be moved to a...ionschema.model.constraintspackages?internal sealed interface SealedConstraint(once we upgrade the language level). Is it worth makingSealedConstraintspublic? If it is public, it would have to be gated by an opt-in annotation because every time we add a new constraint, it would be a breaking change. (See "TODO" comment inConstraint.ktfor more discussion.)Related PRs in ion-schema, ion-schema-tests, ion-schema-schemas:
N/A
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.