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

read safety of external types #1945

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

lsingh123
Copy link
Contributor

@lsingh123 lsingh123 commented Feb 2, 2023

This PR follows up palantir/conjure#1315 to support log safety annotations for external imports, declared at import time.

@changelog-app
Copy link

changelog-app bot commented Feb 2, 2023

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

The generator now supports log safety annotations on external imports, declared at import time.

Check the box to generate changelog(s)

  • Generate changelog entry

@lsingh123 lsingh123 marked this pull request as draft February 2, 2023 21:29
@@ -150,7 +191,7 @@ void testMapSafetyUnsafeValue() {
.types(SAFE_ALIAS)
.types(UNSAFE_ALIAS)
.build();
ConjureDefinitionValidator.validateAll(conjureDef);
ConjureDefinitionValidator.validateAll(conjureDef, SafetyDeclarationRequirements.ALLOWED);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method changed between the previous conjure version and the one I bumped to

SafetyEvaluator evaluator = new SafetyEvaluator(conjureDef);
assertThat(evaluator.evaluate(Iterables.getOnlyElement(conjureDef.getTypes())))
.hasValue(LogSafety.SAFE);
}

private static Stream<Arguments> getTypes(Type externalReference) {
TypeDefinition objectType = TypeDefinition.object(ObjectDefinition.builder()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it's sufficient to just test objects and the rest is overkill

@lsingh123 lsingh123 changed the title external types have safety now read safety of external types Feb 2, 2023
@lsingh123 lsingh123 marked this pull request as ready for review February 2, 2023 22:10
@lsingh123 lsingh123 force-pushed the lsingh/external-imports branch from e2f7b27 to 9751939 Compare February 2, 2023 22:13
@carterkozak
Copy link
Contributor

Could you update example-types.yml to take advantage of this, and regenerate the integrationInput sources using ./gradlew test -Drecreate=true?

Separately (potentially as a separate PR from this one as it may become a little more involved):

I think as written, this will update generated types which contain external imports to reflect the safety of those types, however we probably also want to annotate some of the getter and setter fields with safety annotations since the imported type itself isn't annotated (java.lang.Long doesn't have a safe or unsafe annotation). This way, if we alias a safe external long import, the alias factory method becomes public static MyAlias of(@Safe long value), so that unsafe data cannot be passed in.

@lsingh123
Copy link
Contributor Author

That sounds like a plan - I'll open a separate PR for that change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants