Input deprecation tracker #1064

asteinwedel · 2023-10-12T13:06:24Z

I have a question about accessing arg values, will comment below

asteinwedel · 2023-10-12T13:08:17Z

modules/core/src/main/scala/sangria/execution/FutureResolver.scala

+            // field deprecation
+            val fieldVal: Option[Input] = (ioArg match {
+              case lm: ListMap[String, Input] => lm.get(field.name)
+              case in: Input =>


I'm a little confused about how Args is set at this point

at first I was assuming it was all Input and didn't have the ListMap check here (and didn't have the Seq check below for lists) but that was causing errors

it doesn't appear to actually be Input although everything compiles fine, I had to add a try { } catch { } for None values being here in some tests

removing the input unmarshaller and Input bit entirely seems to actually still work just fine, so think I can just remove that? (going to double check and make a new commit for this)

asteinwedel · 2023-10-12T13:15:27Z

@ #1064 (comment)

latest commit removed the input unmarshalling and everything still seems to work

asteinwedel · 2023-10-12T13:21:38Z

modules/core/src/main/scala/sangria/execution/FutureResolver.scala

+      }
+
+    def trackDeprecatedDirectiveArgs(astDirective: ast.Directive): Unit = {
+      // prevent infinite loop from directiveA -> arg -> directiveA -> arg ...


I'm not sure if this is something we can actually run into with query parsing, but it is something that was possible to run into with tests

modules/core/src/main/scala/sangria/execution/Resolver.scala

modules/core/src/main/scala/sangria/execution/FutureResolver.scala

asteinwedel · 2023-10-12T15:38:10Z

I went ahead and entirely removed the empty deprecation tracker, it didn't seem to serve any purpose now that it's optional and leaving it could have someone unintentionally going through all the tracker work when it's not needed so thought being clear about that would be better

let me know if you want that still in

yanns

It's looking good.
Do you think there's some code I should really focus on?

yanns · 2023-10-12T19:00:04Z

modules/core/src/main/scala/sangria/execution/FutureResolver.scala

@@ -1572,8 +1693,7 @@ private[execution] class FutureResolver[Ctx](
              deferredResolverState = deferredResolverState
            )

-            if (allFields.exists(_.deprecationReason.isDefined))
-              deprecationTracker.deprecatedFieldUsed(ctx)
+            deprecationTracker.map(trackDeprecation(_, ctx))


We're returning Unit, so I think no need to map it.

Suggested change

deprecationTracker.map(trackDeprecation(_, ctx))

deprecationTracker.foreach(trackDeprecation(_, ctx))

habit from mostly working with IO stuff 😅 updating

yanns · 2023-10-12T19:00:38Z

modules/core/src/main/scala/sangria/execution/ValueCoercionHelper.scala

@@ -527,7 +527,7 @@ class ValueCoercionHelper[Ctx](
                  isArgument))),
          { case (v, deprecated) =>
            if (deprecated && userContext.isDefined)
-              deprecationTracker.deprecatedEnumValueUsed(enumT, v, userContext.get)
+              deprecationTracker.map(_.deprecatedEnumValueUsed(enumT, v, userContext.get))


Suggested change

deprecationTracker.map(_.deprecatedEnumValueUsed(enumT, v, userContext.get))

deprecationTracker.foreach(_.deprecatedEnumValueUsed(enumT, v, userContext.get))

asteinwedel · 2023-10-16T07:10:40Z

Do you think there's some code I should really focus on?

sorry for the late response, had a few other things on my plate last week

trackDeprecation is the bulk of it, and tried to cover it thoroughly with the unit tests. The summary is

track directive args (recursively since args can also have directives on them)
track field args (at least this was simple) + their directives+args
track input objects (recursively since fields could be input objects) + their directives+args

yanns

👍 it's looking good, thanks for the tests!

**Dependencies** - Brave 6.0.0 -> 6.0.2 - Brotli4j 1.15.0 -> 1.16.0 - java-control-plane 1.0.42 -> 1.0.44 - Dropwizard 2.1.10 -> 2.1.12 - Dropwizard Metrics 4.2.24 -> 4.2.25 - Eureka 2.0.1 -> 2.0.2 - fastUtil 8.5.12 -> 8.5.13 - gRPC-Java 1.61.0 -> 1.63.0 - Guava 33.0.0-jre -> 33.1.0-jre - Jackson 2.16.1 -> 2.17.0 - JCTools 4.0.2 -> 4.0.3 - Jetty10 10.0.19 -> 10.0.20 - Jetty11 11.0.19 -> 11.0.20 - Jetty12 12.0.5 -> 12.0.8 - Jetty9.4 9.4.52.v20230823 -> 9.4.54.v20240208 - Kotlin 1.9.22 -> 1.9.23 - kotlinx.coroutines 1.9.22 -> 1.9.23 - Kubernetes Client 6.10.0 -> 6.11.0 - Micrometer 1.12.2 -> 1.12.4 - Micrometer Tracing 1.2.2 -> 1.2.4 - Netty 4.1.106.Final -> 4.1.108.Final - Netty io_uring 0.0.24.Final -> 0.0.25.Final - protobuf-jackson 2.2.0 -> 2.5.0 - Reactor 3.6.2 -> 3.6.4 - RESTEasy 5.0.7.Final -> 5.0.9.Final - Retrofit2 2.9.0 -> 2.11.0 - Sangria 4.0.2 -> 4.1.0 - Scala2.12 2.12.18 -> 2.12.19 - Scala2.13 2.13.12 -> 2.13.13 - Scala3 3.3.0 -> 3.4.1 - Spring6 6.1.3 -> 6.1.5 - Spring-Boot3 3.2.2 -> 3.2.4 - Tomcat8 8.5.98 -> 8.5.100 - Tomcat8 9.0.85 -> 9.0.87 - Tomcat8 10.1.18 -> 10.1.20 - Build - Apache HttpClient 5.3 -> 5.3.1 - asm 9.6 -> 9.7 - AssertJ 3.25.2 -> 3.25.3 - Awaitility 4.2.1 -> 4.2.1 - dagger 2.50 -> 2.51.1 - dgs 8.2.2 -> 8.5.3 - Error Prone 2.24.1 -> 2.26.1 - GAX Java 2.40.0 -> 2.46.1 - Gradle 8.5 -> 8.7 - J2ObjC 2.8 -> 3.0.0 - Java-WebSocket 1.5.5 -> 1.5.6 - JUnit5 5.10.1 -> 5.10.2 - Kafka 3.6.1 -> 3.7.0 - krotoDC 1.0.6 -> 1.1.1 - Gradle Nexus Publish Plugin 7.0.1 -> 7.0.2 - SLF4J2 2.0.11 -> 2.0.12 - Testcontainers 1.19.3 -> 1.19.7 - Zookeeper 3.9.1 -> 3.9.2 **Additional Modifications** - `closeAndReleaseStagingRepository` has been renamed to `closeAndReleaseStagingRepositories` - gradle-nexus/publish-plugin#236 - `DnsErrorCauseException` has been introduced in Netty - netty/netty#13721 - `DnsUtil#isDnsQueryTimedOut` has been updated to reflect this - `RefreshingAddressResolver` now uses `DnsUtil#isDnsQueryTimedOut` to determine cachability for consistency - Note that users should sync versions between netty and armeria for this release. Otherwise, a compliation error may occur. - `krotodc` now requires the `protobuf-java-util` dependency - mscheong01/krotoDC#25 - Sangria has a breaking change regarding `DeprecationTracker` - sangria-graphql/sangria#1064

asteinwedel commented Oct 12, 2023

View reviewed changes

asteinwedel force-pushed the input-deprecation-tracker branch 2 times, most recently from acf8842 to f022855 Compare October 12, 2023 13:21

asteinwedel commented Oct 12, 2023

View reviewed changes

asteinwedel force-pushed the input-deprecation-tracker branch from f022855 to 2abad53 Compare October 12, 2023 13:21

yanns reviewed Oct 12, 2023

View reviewed changes

asteinwedel force-pushed the input-deprecation-tracker branch 5 times, most recently from 522d6b0 to 3e87072 Compare October 12, 2023 14:58

asteinwedel requested a review from yanns October 12, 2023 15:18

yanns reviewed Oct 12, 2023

View reviewed changes

asteinwedel force-pushed the input-deprecation-tracker branch from 3e87072 to f3af247 Compare October 16, 2023 07:11

Amanda Steinwedel added 3 commits October 16, 2023 08:11

add input value deprecation tracking

638fab1

make deprecation tracker optional

f0967b7

add breaking changes to build.sbt

95b19c4

asteinwedel force-pushed the input-deprecation-tracker branch from f3af247 to 95b19c4 Compare October 16, 2023 07:11

asteinwedel requested a review from yanns October 16, 2023 07:12

yanns approved these changes Oct 16, 2023

View reviewed changes

yanns added this pull request to the merge queue Oct 16, 2023

Merged via the queue into sangria-graphql:main with commit 6d14691 Oct 16, 2023
4 checks passed

jrhee17 mentioned this pull request Apr 9, 2024

Upgrade dependencies for 1.28.0 line/armeria#5584

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Input deprecation tracker #1064

Input deprecation tracker #1064

asteinwedel commented Oct 12, 2023

asteinwedel Oct 12, 2023

asteinwedel commented Oct 12, 2023 •

edited

Loading

asteinwedel Oct 12, 2023

asteinwedel commented Oct 12, 2023 •

edited

Loading

yanns left a comment

yanns Oct 12, 2023

asteinwedel Oct 16, 2023

yanns Oct 12, 2023

asteinwedel commented Oct 16, 2023 •

edited

Loading

yanns left a comment

	deprecationTracker.map(trackDeprecation(_, ctx))
	deprecationTracker.foreach(trackDeprecation(_, ctx))

	deprecationTracker.map(_.deprecatedEnumValueUsed(enumT, v, userContext.get))
	deprecationTracker.foreach(_.deprecatedEnumValueUsed(enumT, v, userContext.get))

Input deprecation tracker #1064

Input deprecation tracker #1064

Conversation

asteinwedel commented Oct 12, 2023

asteinwedel Oct 12, 2023

Choose a reason for hiding this comment

asteinwedel commented Oct 12, 2023 • edited Loading

asteinwedel Oct 12, 2023

Choose a reason for hiding this comment

asteinwedel commented Oct 12, 2023 • edited Loading

yanns left a comment

Choose a reason for hiding this comment

yanns Oct 12, 2023

Choose a reason for hiding this comment

asteinwedel Oct 16, 2023

Choose a reason for hiding this comment

yanns Oct 12, 2023

Choose a reason for hiding this comment

asteinwedel commented Oct 16, 2023 • edited Loading

yanns left a comment

Choose a reason for hiding this comment

asteinwedel commented Oct 12, 2023 •

edited

Loading

asteinwedel commented Oct 12, 2023 •

edited

Loading

asteinwedel commented Oct 16, 2023 •

edited

Loading