Remove SPI from internal models #796
Conversation
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: fedef1af120d689e0bb8918a |
✅ Deploy Preview for apollo-ios-docc canceled.
|
There was a problem hiding this comment.
Pull Request Overview
This PR removes @_spi annotations from internal models to fix compilation errors. When models use an internal access modifier, SPI annotations should not be included as they are only valid for public declarations.
Key changes:
- Created a new
AccessControlRendererclass to handle both access control modifiers and SPI annotations - Replaced direct access control string generation with centralized renderer throughout templates
- Updated test configurations and expected outputs to match internal access without SPI annotations
Reviewed Changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| AccessControlRenderer.swift | New centralized class for rendering access control with conditional SPI support |
| TemplateRenderer.swift | Refactored to use AccessControlRenderer instead of direct string generation |
| SelectionSetTemplate.swift | Updated to use AccessControlRenderer for consistent access control rendering |
| Various template files | Updated to use new AccessControlRenderer pattern |
| Test configuration files | Updated configurations and expected test outputs |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| init( | ||
| operation: IR.Operation, | ||
| operationIdentifier: String?, | ||
| config: ApolloCodegen.ConfigurationContext, | ||
| ) { |
There was a problem hiding this comment.
Missing parameter documentation for the new initializer. Consider adding documentation comments to explain the parameters and purpose of this initializer.
| @@ -1,13 +0,0 @@ | |||
| // @generated | |||
There was a problem hiding this comment.
What's the reason these files were removed?
There was a problem hiding this comment.
They should never have been checked in in the first place! We aren't customizing our custom scalars. So we actually, want the codegen to regenerate them and test that the newly generated files are correct and still compile.
| fileprivate extension ApolloCodegenConfiguration.AccessModifier { | ||
| var swiftString: String { | ||
| switch self { | ||
| case .public: return "public " // there should be no spaces in these strings |
There was a problem hiding this comment.
The space after "public" is the same as the code was in TemplateRenderer so I assume it had changed over time from the comment, but we should just remove the comment now since it's not applicable.
calvincestari
left a comment
calvincestari
left a comment
There was a problem hiding this comment.
Looks good, nice cleanup to centralize the access control rendered.
Detected SAST Vulnerabilities |
a4e90970f Remove SPI from internal models (#796) git-subtree-dir: apollo-ios-codegen git-subtree-split: a4e90970fa0a1117dd505eaabb7f8265af49fe8f
git-subtree-dir: apollo-ios-codegen git-subtree-mainline: 1aaf87e git-subtree-split: a4e90970fa0a1117dd505eaabb7f8265af49fe8f
Fixes apollographql/apollo-ios#3590
When models use an
internalaccess modifier they should not include@_spinotations. These are only relevant forpublicproperties and will cause a compilation error if included.