From db50d840117aaf05c2354f93566ebf896b95427e Mon Sep 17 00:00:00 2001 From: moiz arafat Date: Thu, 25 Mar 2021 13:41:11 -0400 Subject: [PATCH 1/2] Initial --- .../com/yahoo/elide/async/hooks/AsyncAPIHook.java | 14 ++++++++++---- .../yahoo/elide/async/hooks/AsyncQueryHook.java | 5 +++-- .../yahoo/elide/async/hooks/TableExportHook.java | 5 +++-- .../com/yahoo/elide/async/models/AsyncAPI.java | 2 +- .../main/java/com/yahoo/elide/ElideSettings.java | 1 + .../java/com/yahoo/elide/ElideSettingsBuilder.java | 9 ++++++++- ...ncIntegrationTestApplicationResourceConfig.java | 7 ++++--- .../spring/config/ElideAsyncConfiguration.java | 14 +++++++------- .../spring/config/ElideAutoConfiguration.java | 10 ++++++---- .../standalone/config/ElideResourceConfig.java | 10 ++++------ .../standalone/config/ElideStandaloneSettings.java | 4 ++++ 11 files changed, 51 insertions(+), 30 deletions(-) diff --git a/elide-async/src/main/java/com/yahoo/elide/async/hooks/AsyncAPIHook.java b/elide-async/src/main/java/com/yahoo/elide/async/hooks/AsyncAPIHook.java index d28ad0e3d7..9126efe0fd 100644 --- a/elide-async/src/main/java/com/yahoo/elide/async/hooks/AsyncAPIHook.java +++ b/elide-async/src/main/java/com/yahoo/elide/async/hooks/AsyncAPIHook.java @@ -9,6 +9,8 @@ import static com.yahoo.elide.annotation.LifeCycleHookBinding.Operation.READ; import static com.yahoo.elide.annotation.LifeCycleHookBinding.TransactionPhase.POSTCOMMIT; import static com.yahoo.elide.annotation.LifeCycleHookBinding.TransactionPhase.PRESECURITY; + +import com.yahoo.elide.ElideSettings; import com.yahoo.elide.annotation.LifeCycleHookBinding; import com.yahoo.elide.async.models.AsyncAPI; import com.yahoo.elide.async.models.AsyncAPIResult; @@ -30,11 +32,11 @@ @Data public abstract class AsyncAPIHook implements LifeCycleHook { private final AsyncExecutorService asyncExecutorService; - private final Integer maxAsyncAfterSeconds; + private final ElideSettings elideSettings; - public AsyncAPIHook(AsyncExecutorService asyncExecutorService, Integer maxAsyncAfterSeconds) { + public AsyncAPIHook(AsyncExecutorService asyncExecutorService, ElideSettings elideSettings) { this.asyncExecutorService = asyncExecutorService; - this.maxAsyncAfterSeconds = maxAsyncAfterSeconds; + this.elideSettings = elideSettings; } /** @@ -44,7 +46,7 @@ public AsyncAPIHook(AsyncExecutorService asyncExecutorService, Integer maxAsyncA * @throws InvalidValueException InvalidValueException */ protected void validateOptions(AsyncAPI query, RequestScope requestScope) { - if (query.getAsyncAfterSeconds() > maxAsyncAfterSeconds) { + if (query.getAsyncAfterSeconds() > elideSettings.getMaxAsyncAfterSeconds()) { throw new InvalidValueException("Invalid Async After Seconds"); } } @@ -58,6 +60,10 @@ protected void validateOptions(AsyncAPI query, RequestScope requestScope) { */ protected void executeHook(LifeCycleHookBinding.Operation operation, LifeCycleHookBinding.TransactionPhase phase, AsyncAPI query, RequestScope requestScope, Callable queryWorker) { + if (query.getAsyncAfterSeconds() == null) { + query.setAsyncAfterSeconds(elideSettings.getMaxAsyncAfterSeconds()); + } + if (operation.equals(READ) && phase.equals(PRESECURITY)) { validateOptions(query, requestScope); //We populate the result object when the initial mutation is executed, and then even after executing diff --git a/elide-async/src/main/java/com/yahoo/elide/async/hooks/AsyncQueryHook.java b/elide-async/src/main/java/com/yahoo/elide/async/hooks/AsyncQueryHook.java index c54b7269d5..1ff7ea5e9f 100644 --- a/elide-async/src/main/java/com/yahoo/elide/async/hooks/AsyncQueryHook.java +++ b/elide-async/src/main/java/com/yahoo/elide/async/hooks/AsyncQueryHook.java @@ -5,6 +5,7 @@ */ package com.yahoo.elide.async.hooks; +import com.yahoo.elide.ElideSettings; import com.yahoo.elide.annotation.LifeCycleHookBinding.Operation; import com.yahoo.elide.annotation.LifeCycleHookBinding.TransactionPhase; import com.yahoo.elide.async.models.AsyncAPI; @@ -27,8 +28,8 @@ */ public class AsyncQueryHook extends AsyncAPIHook { - public AsyncQueryHook (AsyncExecutorService asyncExecutorService, Integer maxAsyncAfterSeconds) { - super(asyncExecutorService, maxAsyncAfterSeconds); + public AsyncQueryHook (AsyncExecutorService asyncExecutorService, ElideSettings elideSettings) { + super(asyncExecutorService, elideSettings); } @Override diff --git a/elide-async/src/main/java/com/yahoo/elide/async/hooks/TableExportHook.java b/elide-async/src/main/java/com/yahoo/elide/async/hooks/TableExportHook.java index c87eeb873f..b3e627138d 100644 --- a/elide-async/src/main/java/com/yahoo/elide/async/hooks/TableExportHook.java +++ b/elide-async/src/main/java/com/yahoo/elide/async/hooks/TableExportHook.java @@ -5,6 +5,7 @@ */ package com.yahoo.elide.async.hooks; +import com.yahoo.elide.ElideSettings; import com.yahoo.elide.annotation.LifeCycleHookBinding.Operation; import com.yahoo.elide.annotation.LifeCycleHookBinding.TransactionPhase; import com.yahoo.elide.async.export.formatter.TableExportFormatter; @@ -32,9 +33,9 @@ public class TableExportHook extends AsyncAPIHook { Map supportedFormatters; ResultStorageEngine engine; - public TableExportHook (AsyncExecutorService asyncExecutorService, Integer maxAsyncAfterSeconds, + public TableExportHook (AsyncExecutorService asyncExecutorService, ElideSettings settings, Map supportedFormatters, ResultStorageEngine engine) { - super(asyncExecutorService, maxAsyncAfterSeconds); + super(asyncExecutorService, settings); this.supportedFormatters = supportedFormatters; this.engine = engine; } diff --git a/elide-async/src/main/java/com/yahoo/elide/async/models/AsyncAPI.java b/elide-async/src/main/java/com/yahoo/elide/async/models/AsyncAPI.java index 3a53cf0f0d..29a7bbc6a0 100644 --- a/elide-async/src/main/java/com/yahoo/elide/async/models/AsyncAPI.java +++ b/elide-async/src/main/java/com/yahoo/elide/async/models/AsyncAPI.java @@ -60,7 +60,7 @@ public abstract class AsyncAPI implements PrincipalOwned { @Transient @ComputedAttribute - private Integer asyncAfterSeconds = 10; + private Integer asyncAfterSeconds; /** * Set Async API Result. diff --git a/elide-core/src/main/java/com/yahoo/elide/ElideSettings.java b/elide-core/src/main/java/com/yahoo/elide/ElideSettings.java index 7f021e1a39..bc120ffc90 100644 --- a/elide-core/src/main/java/com/yahoo/elide/ElideSettings.java +++ b/elide-core/src/main/java/com/yahoo/elide/ElideSettings.java @@ -47,4 +47,5 @@ public class ElideSettings { @Getter private final String jsonApiPath; @Getter private final String graphQLApiPath; @Getter private final String exportApiPath; + @Getter private final int maxAsyncAfterSeconds; } diff --git a/elide-core/src/main/java/com/yahoo/elide/ElideSettingsBuilder.java b/elide-core/src/main/java/com/yahoo/elide/ElideSettingsBuilder.java index c3633d1378..c29e21551c 100644 --- a/elide-core/src/main/java/com/yahoo/elide/ElideSettingsBuilder.java +++ b/elide-core/src/main/java/com/yahoo/elide/ElideSettingsBuilder.java @@ -57,6 +57,7 @@ public class ElideSettingsBuilder { private String jsonApiPath; private String graphQLApiPath; private String exportApiPath; + private int maxAsyncAfterSeconds = 10; /** * A new builder used to generate Elide instances. Instantiates an {@link EntityDictionary} without @@ -112,7 +113,8 @@ public ElideSettings build() { baseUrl, jsonApiPath, graphQLApiPath, - exportApiPath); + exportApiPath, + maxAsyncAfterSeconds); } public ElideSettingsBuilder withAuditLogger(AuditLogger auditLogger) { @@ -221,4 +223,9 @@ public ElideSettingsBuilder withStrictQueryParams(boolean enabled) { this.strictQueryParams = enabled; return this; } + + public ElideSettingsBuilder withMaxAsyncAfterSeconds(int maxAsyncAfterSeconds) { + this.maxAsyncAfterSeconds = maxAsyncAfterSeconds; + return this; + } } diff --git a/elide-integration-tests/src/test/java/com/yahoo/elide/async/integration/tests/framework/AsyncIntegrationTestApplicationResourceConfig.java b/elide-integration-tests/src/test/java/com/yahoo/elide/async/integration/tests/framework/AsyncIntegrationTestApplicationResourceConfig.java index 3f28a307cb..5cc5b43508 100644 --- a/elide-integration-tests/src/test/java/com/yahoo/elide/async/integration/tests/framework/AsyncIntegrationTestApplicationResourceConfig.java +++ b/elide-integration-tests/src/test/java/com/yahoo/elide/async/integration/tests/framework/AsyncIntegrationTestApplicationResourceConfig.java @@ -101,6 +101,7 @@ protected void configure() { .withEntityDictionary(dictionary) .withISO8601Dates("yyyy-MM-dd'T'HH:mm'Z'", Calendar.getInstance().getTimeZone()) .withExportApiPath("/export") + .withMaxAsyncAfterSeconds(10) .build()); bind(elide).to(Elide.class).named("elide"); @@ -123,8 +124,8 @@ protected void configure() { supportedFormatters.put(ResultType.JSON, new JSONExportFormatter(elide)); // Binding TableExport LifeCycleHook - TableExportHook tableExportHook = new TableExportHook(asyncExecutorService, 10, supportedFormatters, - resultStorageEngine); + TableExportHook tableExportHook = new TableExportHook(asyncExecutorService, elide.getElideSettings(), + supportedFormatters, resultStorageEngine); dictionary.bindTrigger(TableExport.class, READ, PRESECURITY, tableExportHook, false); dictionary.bindTrigger(TableExport.class, CREATE, POSTCOMMIT, tableExportHook, false); dictionary.bindTrigger(TableExport.class, CREATE, PRESECURITY, tableExportHook, false); @@ -143,7 +144,7 @@ public long purchase(Invoice invoice) { bind(billingService).to(BillingService.class); // Binding AsyncQuery LifeCycleHook - AsyncQueryHook asyncQueryHook = new AsyncQueryHook(asyncExecutorService, 10); + AsyncQueryHook asyncQueryHook = new AsyncQueryHook(asyncExecutorService, elide.getElideSettings()); InvoiceCompletionHook invoiceCompletionHook = new InvoiceCompletionHook(billingService); diff --git a/elide-spring/elide-spring-boot-autoconfigure/src/main/java/com/yahoo/elide/spring/config/ElideAsyncConfiguration.java b/elide-spring/elide-spring-boot-autoconfigure/src/main/java/com/yahoo/elide/spring/config/ElideAsyncConfiguration.java index c49c945d4c..2007119b64 100644 --- a/elide-spring/elide-spring-boot-autoconfigure/src/main/java/com/yahoo/elide/spring/config/ElideAsyncConfiguration.java +++ b/elide-spring/elide-spring-boot-autoconfigure/src/main/java/com/yahoo/elide/spring/config/ElideAsyncConfiguration.java @@ -11,6 +11,7 @@ import static com.yahoo.elide.annotation.LifeCycleHookBinding.TransactionPhase.PRESECURITY; import com.yahoo.elide.Elide; +import com.yahoo.elide.ElideSettings; import com.yahoo.elide.async.export.formatter.CSVExportFormatter; import com.yahoo.elide.async.export.formatter.JSONExportFormatter; import com.yahoo.elide.async.export.formatter.TableExportFormatter; @@ -74,8 +75,7 @@ public AsyncExecutorService buildAsyncExecutorService(Elide elide, ElideConfigPr AsyncExecutorService asyncExecutorService = new AsyncExecutorService(elide, executor, updater, asyncQueryDao); // Binding AsyncQuery LifeCycleHook - AsyncQueryHook asyncQueryHook = new AsyncQueryHook(asyncExecutorService, - asyncProperties.getMaxAsyncAfterSeconds()); + AsyncQueryHook asyncQueryHook = new AsyncQueryHook(asyncExecutorService, elide.getElideSettings()); dictionary.bindTrigger(AsyncQuery.class, READ, PRESECURITY, asyncQueryHook, false); dictionary.bindTrigger(AsyncQuery.class, CREATE, POSTCOMMIT, asyncQueryHook, false); dictionary.bindTrigger(AsyncQuery.class, CREATE, PRESECURITY, asyncQueryHook, false); @@ -109,14 +109,14 @@ private TableExportHook getTableExportHook(AsyncExecutorService asyncExecutorSer ElideConfigProperties settings, Map supportedFormatters, ResultStorageEngine resultStorageEngine) { boolean exportEnabled = ElideAutoConfiguration.isExportEnabled(settings.getAsync()); - + ElideSettings elideSettings = asyncExecutorService.getElide().getElideSettings(); TableExportHook tableExportHook = null; if (exportEnabled) { - tableExportHook = new TableExportHook(asyncExecutorService, settings.getAsync().getMaxAsyncAfterSeconds(), - supportedFormatters, resultStorageEngine); + tableExportHook = new TableExportHook(asyncExecutorService, elideSettings, supportedFormatters, + resultStorageEngine); } else { - tableExportHook = new TableExportHook(asyncExecutorService, settings.getAsync().getMaxAsyncAfterSeconds(), - supportedFormatters, resultStorageEngine) { + tableExportHook = new TableExportHook(asyncExecutorService, elideSettings, supportedFormatters, + resultStorageEngine) { @Override public void validateOptions(AsyncAPI export, RequestScope requestScope) { throw new InvalidOperationException("TableExport is not supported."); diff --git a/elide-spring/elide-spring-boot-autoconfigure/src/main/java/com/yahoo/elide/spring/config/ElideAutoConfiguration.java b/elide-spring/elide-spring-boot-autoconfigure/src/main/java/com/yahoo/elide/spring/config/ElideAutoConfiguration.java index f73ab3963b..e637f20396 100644 --- a/elide-spring/elide-spring-boot-autoconfigure/src/main/java/com/yahoo/elide/spring/config/ElideAutoConfiguration.java +++ b/elide-spring/elide-spring-boot-autoconfigure/src/main/java/com/yahoo/elide/spring/config/ElideAutoConfiguration.java @@ -150,10 +150,12 @@ public Elide initializeElide(EntityDictionary dictionary, .withJsonApiPath(settings.getJsonApi().getPath()) .withGraphQLApiPath(settings.getGraphql().getPath()); - if (settings.getAsync() != null - && settings.getAsync().getExport() != null - && settings.getAsync().getExport().isEnabled()) { - builder.withExportApiPath(settings.getAsync().getExport().getPath()); + if (settings.getAsync() != null) { + builder.withMaxAsyncAfterSeconds(settings.getAsync().getMaxAsyncAfterSeconds()); + + if (settings.getAsync().getExport() != null && settings.getAsync().getExport().isEnabled()) { + builder.withExportApiPath(settings.getAsync().getExport().getPath()); + } } if (settings.getJsonApi() != null diff --git a/elide-standalone/src/main/java/com/yahoo/elide/standalone/config/ElideResourceConfig.java b/elide-standalone/src/main/java/com/yahoo/elide/standalone/config/ElideResourceConfig.java index c7b4f6734e..45ee3ef9dc 100644 --- a/elide-standalone/src/main/java/com/yahoo/elide/standalone/config/ElideResourceConfig.java +++ b/elide-standalone/src/main/java/com/yahoo/elide/standalone/config/ElideResourceConfig.java @@ -196,8 +196,7 @@ protected void configure() { } // Binding AsyncQuery LifeCycleHook - AsyncQueryHook asyncQueryHook = new AsyncQueryHook(asyncExecutorService, - asyncProperties.getMaxAsyncAfterSeconds()); + AsyncQueryHook asyncQueryHook = new AsyncQueryHook(asyncExecutorService, elideSettings); dictionary.bindTrigger(AsyncQuery.class, READ, PRESECURITY, asyncQueryHook, false); dictionary.bindTrigger(AsyncQuery.class, CREATE, POSTCOMMIT, asyncQueryHook, false); @@ -273,13 +272,12 @@ public static HealthCheckRegistry getHealthCheckRegistry() { private TableExportHook getTableExportHook(AsyncExecutorService asyncExecutorService, ElideStandaloneAsyncSettings asyncProperties, Map supportedFormatters, ResultStorageEngine engine) { + ElideSettings elideSettings = asyncExecutorService.getElide().getElideSettings(); TableExportHook tableExportHook = null; if (asyncProperties.enableExport()) { - tableExportHook = new TableExportHook(asyncExecutorService, asyncProperties.getMaxAsyncAfterSeconds(), - supportedFormatters, engine); + tableExportHook = new TableExportHook(asyncExecutorService, elideSettings, supportedFormatters, engine); } else { - tableExportHook = new TableExportHook(asyncExecutorService, asyncProperties.getMaxAsyncAfterSeconds(), - supportedFormatters, engine) { + tableExportHook = new TableExportHook(asyncExecutorService, elideSettings, supportedFormatters, engine) { @Override public void validateOptions(AsyncAPI export, RequestScope requestScope) { throw new InvalidOperationException("TableExport is not supported."); diff --git a/elide-standalone/src/main/java/com/yahoo/elide/standalone/config/ElideStandaloneSettings.java b/elide-standalone/src/main/java/com/yahoo/elide/standalone/config/ElideStandaloneSettings.java index 9e09a65a70..a2f563302b 100644 --- a/elide-standalone/src/main/java/com/yahoo/elide/standalone/config/ElideStandaloneSettings.java +++ b/elide-standalone/src/main/java/com/yahoo/elide/standalone/config/ElideStandaloneSettings.java @@ -127,6 +127,10 @@ default ElideSettings getElideSettings(EntityDictionary dictionary, DataStore da builder.withExportApiPath(getAsyncProperties().getExportApiPathSpec().replaceAll("/\\*", "")); } + if (getAsyncProperties().getMaxAsyncAfterSeconds() != null) { + builder.withMaxAsyncAfterSeconds(getAsyncProperties().getMaxAsyncAfterSeconds()); + } + if (enableISO8601Dates()) { builder.withISO8601Dates("yyyy-MM-dd'T'HH:mm'Z'", TimeZone.getTimeZone("UTC")); } From ce8c7586fd90904e7183bfbcac00d32f8d978fe0 Mon Sep 17 00:00:00 2001 From: moiz arafat Date: Thu, 25 Mar 2021 15:37:49 -0400 Subject: [PATCH 2/2] review comment --- .../yahoo/elide/async/hooks/AsyncAPIHook.java | 27 ++++++++++++++----- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/elide-async/src/main/java/com/yahoo/elide/async/hooks/AsyncAPIHook.java b/elide-async/src/main/java/com/yahoo/elide/async/hooks/AsyncAPIHook.java index 9126efe0fd..320f3b2baf 100644 --- a/elide-async/src/main/java/com/yahoo/elide/async/hooks/AsyncAPIHook.java +++ b/elide-async/src/main/java/com/yahoo/elide/async/hooks/AsyncAPIHook.java @@ -60,16 +60,29 @@ protected void validateOptions(AsyncAPI query, RequestScope requestScope) { */ protected void executeHook(LifeCycleHookBinding.Operation operation, LifeCycleHookBinding.TransactionPhase phase, AsyncAPI query, RequestScope requestScope, Callable queryWorker) { - if (query.getAsyncAfterSeconds() == null) { - query.setAsyncAfterSeconds(elideSettings.getMaxAsyncAfterSeconds()); - } if (operation.equals(READ) && phase.equals(PRESECURITY)) { + // AsyncAfterSeconds is a transient property. When we update the STATUS of an AsyncRequest for + // CANCELLATION we do not have the property set. This fails the "validateOptions" method call. + // Below code sets the value based on ElideSettings if its null. + if (query.getAsyncAfterSeconds() == null) { + query.setAsyncAfterSeconds(elideSettings.getMaxAsyncAfterSeconds()); + } + validateOptions(query, requestScope); - //We populate the result object when the initial mutation is executed, and then even after executing - //the hooks we return the same object back. QueryRunner.java#L190. - //In GraphQL, the only part of the body that is lazily returned is the ID. - //ReadPreSecurityHook - Those hooks get evaluated in line with the request processing. + + // Grapqhl AsyncRequest which were submitted with non-zero asyncAfterSeconds were not returning correct + // Status. They came back with status QUEUED instead of COMPLETE. + // Root Cause: + // We populate the result object when the initial mutation is executed, and then even after executing + // the hooks we return the same object back. https://github.com/yahoo/elide/blob/ + // d4901c6f07e57aa179c5afd640c9c67e90a8cdaf/elide-graphql/src/main/java/com/yahoo/elide/graphql/ + // QueryRunner.java#L190. In GraphQL, the only part of the body that is lazily returned is the ID. + // Solution: + // We decided to implement the execution flow as a ReadPreSecurityHook. + // Those hooks get evaluated in line with the request processing. + // Every time its read, we check if the STATUS is Queued AND Result is NULL, then we continue with the + // Asynchronous execution call else we exit from the async flow. executeAsync(query, queryWorker); return; }