-
Notifications
You must be signed in to change notification settings - Fork 159
While, Do-while loop implementation for fetching paginated data from an API #1096
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
Comments
However, It appears that the current By setting the Example:
Example:
|
May I know if
According to typescript SDK it's optional, but its no-where that's mentioned in the specs, can you clarify please? |
@nyamathshaik Yes, Therefore, you initial proposal looks to me to be the most adequate, and would also restore a construct we had in previous versions. |
@cdavernas Thank you for the feedback. I was considering something along the lines of the following, but I am still open to suggestions. Please review it and let me know if it looks good.
Benefits of Proposed Approach
This proposed solution provides a structured approach to implementing loop tasks while maintaining compatibility with Serverless Workflow specifications. |
@nyamathshaik First of all, thanks for your awesome work! Second, I have a couple of remarks regarding the proposal you made in related PR: do:
- fetchPaginatedData:
while: .hasNextPage == true
postConditionCheck: false # Enables do-while behavior
maxIterations: 100
do:
- fetchPage:
call: getPageData
input:
pageToken: .nextPageToken
output: .fetchedData
- accumulateData:
run: mergeResults
input:
newData: .fetchedData
existingData: .accumulatedResults
output: .accumulatedResults
|
@cdavernas Thanks a lot for the feedback and kind words! 🙌 Really appreciate you taking the time to review the proposal. Totally hear you on the concerns around the For the keyword conflict: I’m happy to revise the structure to avoid any ambiguity with
Open to suggestions! I'm happy to iterate further on the proposal based on whatever naming conventions or structural guidance the maintainers feel would fit best. Looking forward to your thoughts! |
or how about just simply renaming the initial proposal to below :
|
Why not: - name: WhileLoopExample
while: ${ .counter < 5 }
do:
- name: IncrementCounter
type: set
data:
counter: ${ .counter + 1 }
- name: LogMessage
type: run
run:
script:
lang: javascript
code: |
console.log("Counter is: " + context.counter); And - name: WhileLoopExample
do:
- name: IncrementCounter
type: set
data:
counter: ${ .counter + 1 }
- name: LogMessage
type: run
run:
script:
lang: javascript
code: |
console.log("Counter is: " + context.counter);
while: ${ .counter < 5 } The challenge is to interpret this in the JSON Schema. Essentially, you want a |
We can add the |
The first is a |
Thanks for reviewing @ricardozanini JSON Schema doesn’t support positional logic. So I don't think comment actually works. (i.e., if while appears before, interpret it one way; if after, interpret differently). Also, this creates more Ambiguity in Semantics (while appearing before or after do: changes the behavior). |
That said, I am still aligned with below : Approach 1:
Approach 2:
Please let me know if you have any further suggestions, or an entire new suggestion is fine too. So that we can agree upon the final solution and I can work on raising a PR. |
@nyamathshaik Am I missing something or approach 1 and 2 are the exact same in above comment? Otherwise, IMHO the last suggestions are far better than the initial one, on a semantic point of view. I'm not a big fan of the |
@cdavernas @nyamathshaik Golang doesn't have https://www.programiz.com/golang/while-loop I'm not a fan of this |
@ricardozanini The problem is that it would mean to also remove the To conclude, I think @nyamathshaik is right to (re)introduce (we had that before 1.x.x) a new dedicated task. Motivations are:
|
Thanks alot for taking time and reviewing this request @ricardozanini @cdavernas I agree with @cdavernas Removing the That said, we have below 3 approaches to finalize from. Please help confirm. Approach 1:
Approach 2:
As per @ricardozanini suggestion Approach 3:
|
I think Zanini proposal is the most intuitive one. To surpass the json schema challenge, we can replace the while: by repeatWhile:, so they look this way Standard while task
Do while
|
@fjtirado As explained above, @ricardozanini proposal is not doable, as it would either force the removal of the |
I do not think so, I bet you can have a while task and a for task with a while. What you cannot have is two whiles at the same level in different possition, thats why I replaced one of Ricardos while by a repeatWhile. |
Not really: in one case |
@fjtirado If we want to do @ricardozanini approach, we will have to use the existing while property inside However, @ricardozanini also suggested if we can just make Adding a new task type is the easiest and safest route we can take IMHO also. |
Ok, let me rephrase, what I suggested (please take a closer look to the example I use) is a new while task for regular while. |
@fjtirado The while keyword will need to change, either in the task you propose to add, or in the for task, or discrimination will no longer be (easily) possible (i.e. check for presence of while keyword, which as a primary identifier must not be use by any other task) |
With that said, we now have 4 approaches from which we need to finalize from? @ricardozanini @cdavernas @fjtirado My Preferance is Approach 2, followed by 1. WDYT? ✅ Approach 1: Using explicit loop: while | do-while keyword
✅ Approach 2: Using condition as task and isDoWhile as a flag to determine if its while or a do-while loop
✅ Approach 3: Using current while property from
✅ Approach 4: Having 2 different tasks, Using current while property from
|
I'd hate to add another task to do loops since we already have one. A: document:
dsl: '1.0.0'
namespace: default
name: for
version: '1.0.0'
do:
- myDoLoop
for:
while: ${ .counter < 5 }
- name: IncrementCounter
type: set
data:
counter: ${ .counter + 1 }
- name: LogMessage
type: run
run:
script:
lang: javascript
code: console.log("Counter is: " + context.counter); Won't break anything and will add the possibility to keep the existing IF we were to add a new task, neither of the approaches is compelling to me. What we can do to keep the same philosofy is: document:
dsl: '1.0.0'
namespace: default
name: while
version: '1.0.0'
do:
- myDoLoop
while: ${ .counter < 5 }
do:
- name: IncrementCounter
type: set
data:
counter: ${ .counter + 1 }
- name: LogMessage
type: run
run:
script:
lang: javascript
code: console.log("Counter is: " + context.counter); And repeat: document:
dsl: '1.0.0'
namespace: default
name: repeat
version: '1.0.0'
do:
- myDoLoop
repeat:
do:
- name: IncrementCounter
type: set
data:
counter: ${ .counter + 1 }
- name: LogMessage
type: run
run:
script:
lang: javascript
code: console.log("Counter is: " + context.counter);
until: ${ .counter < 5 } Yes, we will introduce two additional tasks, but it's closer to the language philosophy we have now, and get rid of these boolean/enum attributes that are not fluent. |
@nyamathshaik regarding this:
Yes, it would work because the |
@ricardozanini This sounds good to me. But wouldn't the
And repeat:
|
@cdavernas @ricardozanini Can you guys please confirm on the final proposal please? So I can go ahead and work on the PR please. |
@cdavernas will probably review this tomorrow. |
@cdavernas any update on this please? |
@nyamathshaik I need more time to look at alternatives, and my hands are full today. I'll address whenever possible! In the meantime, @JBBianchi, WDYT? |
This is an interesting proposal. I’m just wondering if it’s truly necessary to introduce a new construct for this, since the current spec already supports looping behavior using a simple combination of Here's an example of a "self loop" (essentially mimicking a document:
dsl: "1.0.0-alpha5"
namespace: default
name: do-while
version: "0.1.0"
do:
- loop:
if: ${ .done != true }
set:
foo: ${ (.foo//0) + 1 }
done: ${ .foo == 5 }
then: loop
- next:
set:
foo: ${ .foo }
bar: "bar" This approach keeps things simple and avoids adding more complexity to the spec. That said, if the primary motivation is to improve readability or reduce boilerplate for common patterns like pagination, I totally get the intent. Maybe it's worth exploring ways to document or abstract this pattern rather than extending the spec? Curious to hear others’ thoughts! |
@JBBianchi This is more a while rather than a do while, isnt it? |
@JBBianchi I believe currently we don't support That's my initial alternate solution as well can check here But the problem with this approach is : It is harder to read, maintain, and error-prone when retry logic and timeouts are introduced. WDYT? |
If is on the spec and together with then (which I forgot) allows for a clear loop in my opinion. It is not a do while though, but a while |
@nyamathshaik We do support
@fjtirado Is that a deal-breaker? |
@cdavernas I might be wrong, but I think all do whiles can be implemented as while at the cost of complicating the while (in this case the if) condition a bit. Not a deal breaker in my opinion. |
@cdavernas @fjtirado I agree that @JBBianchi’s approach effectively handles both while and do-while loop scenarios, and as noted, it’s already listed among the alternative approaches in the related issue. That said, this method can become difficult to read and maintain—especially when introducing retry logic, timeouts, nested loops, or early exits. These additions tend to increase complexity and the likelihood of errors. That being said, I’m also comfortable proceeding with @ricardozanini’s approach. @cdavernas, could we move toward a final decision on this? |
@cdavernas Can you help review this comment please? |
As said before, I'll come back to you whenever possible, my hands are full for the time being. I love the enthusiasm, but I'd like to give that issue the proper care and attention, rushing into solutions is but rarely productive in a spec's context. I'm sorry to have to make you wait a bit more, mate. No need to ping me to accelerate the process, I really don't have time now, but I'll probably be able to address it by tomorrow morning 😉 In the time being, @matthias-pichler @geomagilles, care to add your grain of salt? |
Agreed with @cdavernas. This type of change is always challenging, and what you are asking, @nyamathshaik, is not a simple decision. It will impact the spec's future since, if for some reason we deprecate these additional tasks, it would require a new major version in the future. Although it looks simple on paper, it's not. Given that you can achieve the same with an empty |
Thanks @cdavernas and @ricardozanini — I really appreciate the thoughtful responses and completely understand the need to approach spec changes with care and long-term impact in mind. I’ll hold off for now and look forward to further discussion once you have the bandwidth. In the meantime, happy to hear any thoughts from @matthias-pichler or @geomagilles as well. Thanks again for taking the time despite your packed schedules! |
Hi, thanks for raising this. I don't have a strong opinion on the specific proposal, but I agree that such decisions shouldn't be rushed, as previously mentioned. One way to support both users and maintainers is by supplementing the documentation with practical "how-to" guides. These guides can help end-users better understand and apply the specification, and they can also provide maintainers with insights into areas where users may encounter difficulties due to complexity. |
Hi, @ricardozanini @cdavernas any update on this please? Also, can you confirm if we had to make changes should we also make changes on the SDKs or are they auto generated? |
@nyamathshaik Once the spec schema is updated, a new version of the SDKs incoporating that schema change will be released. |
@ricardozanini @cdavernas Can I request to update |
What would you like to be added?
Support for while or do-while loop constructs to simplify workflows that need to fetch paginated data from APIs until a condition is met (e.g., no more pages to fetch).
Proposal(s):
Currently, implementing workflows that require repetitive API calls until a condition is met (e.g., while nextPageToken is present) is cumbersome and verbose using the existing specification. There is no native support for loop constructs that check a condition after each iteration (i.e., a do-while pattern).
Proposal:
Example syntax (pseudo-DSL):
Alternative(s):
Currently, the same can be achieved by:
transition
and using aswitch
to re-enter a state based on a condition.workflow calls
, which increases complexity and reduces readability.These approaches are harder to read, maintain, and error-prone when retry logic and timeouts are introduced.
Additional info:
This feature would greatly improve:
Let me know if you'd like me to open a PR to help explore this idea! Or do you think this can be achieved with current For loop? @ricardozanini @cdavernas
Community Notes
The text was updated successfully, but these errors were encountered: