Events and API unification - Market API. #52

stranger80 · 2020-10-24T13:32:18Z

This is part I of REST API unification and event-handling changes - Market API.

Part II is #53

Confirmed to be back-compatible with yapapi v0.3.0 via golemfactory/yagna#758
and later on via golemfactory/yagna#775

tworec

the same set of comments/changes as in second part of my review for #53 applies here also

specs/common.yaml

specs/market-api.yaml

…sistency

… consistency

nieznanysprawiciel

Second part of review (last)

nieznanysprawiciel · 2020-11-04T15:17:34Z

model/src/market/event.rs

+        proposal: Proposal,
+    },
+    #[serde(rename = "ProposalRejectedEvent")]
+    ProposalRejectedEvent {


To consider:
Since we have PropsalRejectedEvent, maybe we could give better name to ProposalEvent whose name seems to be to general now. Probably similar to AgreementEvent.

Second thing to consider:
I saw in rust Requestor code, that when querying Proposals, he must check if Proposal is initial and make different decisions based on this. Maybe it's reason to separate initial Proposals from counter Proposals??

We can't change event names and semantics for Alpha.3, because we need to be backward compatible.
And this PR is meant for Alpha.3

postponed to #53

model/src/market/agreement_event.rs

model/src/market/demand_offer_base.rs

specs/market-api.yaml

* Reason message is required, but Reason itself is optional * removed AgreementTimeoutEvent * renamed operations from `rejectProposal__Reason` to `rejectProposal__WithReason` * restored double linebreaks in descriptions to render paragraphs properly in generated documentation

model/src/market/reason.rs

…ason in client

tworec · 2020-11-09T09:08:15Z

specs/market-api.yaml

      responses:
-        '200':
+        200:
          description: Agreement approval result.
          content:
            application/json:
              schema:
                type: string
                enum: [Approved, Rejected, Cancelled]


@nieznanysprawiciel the solution for your concern is to return here a single event, but then we will not be back compatible, so it is not possible in this PR which is meant for Alpha.3

$ref: '#/components/schemas/AgreementEvent'

and in ApproveAgreement on Provider

postponed to #53

tworec · 2020-11-09T09:16:13Z

specs/market-api.yaml

+      - $ref: 'common.yaml#/parameters/pollTimeout'
+      - $ref: 'common.yaml#/parameters/afterTimestamp'
+      - $ref: 'common.yaml#/parameters/maxEvents'
+      - $ref: 'common.yaml#/parameters/appSessionId'


To enable querying only for events regarding single agreement

Suggested change

- $ref: 'common.yaml#/parameters/appSessionId'

- $ref: 'common.yaml#/parameters/appSessionId'

- $ref: '#/components/parameters/queryAgreementId'

tworec · 2020-11-09T09:21:09Z

specs/market-api.yaml

@@ -827,27 +967,33 @@ components:



To enable querying only for events regarding a single agreement

Suggested change

queryAgreementId:

name: agreementId

description: Used to enable querying agreement events related to a single agreement.

in: query

required: false

schema:

type: string

For sure it is worth considering, because we have no easy way to query, for example, termination reason for specific Agreement, without querying all events, even these unrelated.
But note that adding new optional filter to endpoint is backward compatible change, so I suggest to postpone these changes for later and investigate other options.

postponed to #53

tworec

marked all comments postponed to #53 to easily find them when we need

tworec · 2020-11-09T11:03:16Z

specs/market-api.yaml

@@ -827,27 +967,33 @@ components:



postponed to #53

tworec · 2020-11-09T11:03:29Z

specs/market-api.yaml

      responses:
-        '200':
+        200:
          description: Agreement approval result.
          content:
            application/json:
              schema:
                type: string
                enum: [Approved, Rejected, Cancelled]


postponed to #53

tworec · 2020-11-09T11:04:46Z

model/src/market/event.rs

+        proposal: Proposal,
+    },
+    #[serde(rename = "ProposalRejectedEvent")]
+    ProposalRejectedEvent {


postponed to #53

Events and API unification - Market API.

1f20fee

stranger80 requested review from nieznanysprawiciel and tworec October 24, 2020 13:33

MBarwicki added 3 commits October 24, 2020 15:35

Trim trailing whitespaces.

2f02945

No newline at end of file.

5c8a833

Pre-commit ran.

7f58ff6

stranger80 mentioned this pull request Oct 24, 2020

[BB10] Event API Draft #2

Closed

tworec suggested changes Oct 26, 2020

View reviewed changes

specs/common.yaml Show resolved Hide resolved

MBarwicki and others added 7 commits October 27, 2020 23:21

Review comments & updates

7931a26

Merge branch 'master' into stranger80/market-api-events

c73279e

pre-commit

bcfc8a1

Using DemandOfferBase as input to Subscribe calls

f255ee0

Using DemandOfferBase as input to CounterProposal

5ebc2ad

back compatibility for reject proposal

764042d

remove unused: 410/Gone from agreement events and AgreementProposalEvent

8fc1b6b

stranger80 commented Nov 2, 2020

View reviewed changes

specs/market-api.yaml Outdated Show resolved Hide resolved

nieznanysprawiciel reviewed Nov 3, 2020

View reviewed changes

specs/market-api.yaml Outdated Show resolved Hide resolved

nieznanysprawiciel reviewed Nov 3, 2020

View reviewed changes

specs/market-api.yaml Show resolved Hide resolved

nieznanysprawiciel reviewed Nov 3, 2020

View reviewed changes

specs/market-api.yaml Show resolved Hide resolved

tworec added 6 commits November 3, 2020 13:52

[mkt spec] AgreementEvent back compatibility

e123906

[mkt spec] named types fo Timestamp and ValidTo + required fields con…

dfb24af

…sistency

[mkt spec] bump version

86d25ea

[mkt spec] restored date-time to enable description + required fields…

83cc60c

… consistency

[mkt spec] dedent to enable proper code generation

7ce47ef

[mkt] client code compliant with 1.6.1 spec

c5c72e9

tworec mentioned this pull request Nov 3, 2020

Events and API unification - Market, Activity & Payment API #53

Merged

tworec added 4 commits November 3, 2020 16:43

[mkt spec] apply review comments

6604805

[mkt] Event: align generated code with yaml spec

932869d

[mkt spec] remove AgreementTimeoutEvent + minor tweaks

261f6ce

[mkt spec] backward compatibility for query params

2c7b6c4

tworec removed a link to an issue Nov 4, 2020

Market unsubscribe result type #21

Closed

nieznanysprawiciel reviewed Nov 4, 2020

View reviewed changes

nieznanysprawiciel reviewed Nov 5, 2020

View reviewed changes

specs/market-api.yaml Show resolved Hide resolved

tworec force-pushed the stranger80/market-api-events branch from 98ee654 to 5738047 Compare November 5, 2020 20:13

tworec added 4 commits November 5, 2020 21:17

[mkt] use aliases instead of DemandOfferBase

44df81c

[mkt] add appSessionId to the Agreement

e6d5a2d

[mkt] terminatedSignature added to Agreement

27bdc52

tworec force-pushed the stranger80/market-api-events branch from 5738047 to 27bdc52 Compare November 5, 2020 20:25

nieznanysprawiciel reviewed Nov 6, 2020

View reviewed changes

model/src/market/reason.rs Outdated Show resolved Hide resolved

tworec added 7 commits November 6, 2020 14:39

Merge branch 'master' into stranger80/market-api-events

b6ce271

[mkt] model back compatiblity - pub use DemandOfferBase

9a35d94

[mkt] Reason to support extra properties

50f1897

[mkt] added cancelAgreementWithReason in spec + all endpoints with re…

25da200

…ason in client

[mkt] termination signature moved from Agreement to Event

9cb09d0

[mkt] **All** Agreement events will be populated on **both** sides

28cef62

[mkt] improoved descriptions for some endpoints

ba89a54

tworec reviewed Nov 9, 2020

View reviewed changes

nieznanysprawiciel approved these changes Nov 9, 2020

View reviewed changes

tworec reviewed Nov 9, 2020

View reviewed changes

tworec approved these changes Nov 9, 2020

View reviewed changes

tworec mentioned this pull request Nov 9, 2020

Adapt to new Market Api 1.6.1 golemfactory/yagna#775

Merged

tworec merged commit fe426ad into master Nov 9, 2020

Mkt & Prov (Yagna Core) automation moved this from In progress to Done Nov 9, 2020

tworec deleted the stranger80/market-api-events branch November 9, 2020 13:35

tworec restored the stranger80/market-api-events branch November 10, 2020 13:02

tworec mentioned this pull request Nov 10, 2020

cancel_agreement marked as deprecated #62

Merged

nieznanysprawiciel moved this from Done to Archive in Mkt & Prov (Yagna Core) Nov 12, 2020

tworec deleted the stranger80/market-api-events branch December 11, 2020 10:53

tworec moved this from Archive to Done in Mkt & Prov (Yagna Core) Jan 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Events and API unification - Market API. #52

Events and API unification - Market API. #52

stranger80 commented Oct 24, 2020 •

edited by tworec

Loading

tworec left a comment

nieznanysprawiciel left a comment

nieznanysprawiciel Nov 4, 2020

nieznanysprawiciel Nov 4, 2020

tworec Nov 5, 2020

tworec Nov 9, 2020

tworec Nov 9, 2020

nieznanysprawiciel Nov 9, 2020 •

edited

Loading

tworec Nov 9, 2020

tworec Nov 9, 2020 •

edited

Loading

tworec Nov 9, 2020

nieznanysprawiciel Nov 9, 2020 •

edited

Loading

tworec Nov 9, 2020

tworec left a comment

tworec Nov 9, 2020

tworec Nov 9, 2020

tworec Nov 9, 2020

	- $ref: 'common.yaml#/parameters/appSessionId'
	- $ref: 'common.yaml#/parameters/appSessionId'
	- $ref: '#/components/parameters/queryAgreementId'

+    queryAgreementId:
+      name: agreementId
+      description: Used to enable querying agreement events related to a single agreement.
+      in: query
+      required: false
+      schema:
+        type: string

Events and API unification - Market API. #52

Events and API unification - Market API. #52

Conversation

stranger80 commented Oct 24, 2020 • edited by tworec Loading

tworec left a comment

Choose a reason for hiding this comment

nieznanysprawiciel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nieznanysprawiciel Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tworec Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nieznanysprawiciel Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tworec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stranger80 commented Oct 24, 2020 •

edited by tworec

Loading

nieznanysprawiciel Nov 9, 2020 •

edited

Loading

tworec Nov 9, 2020 •

edited

Loading

nieznanysprawiciel Nov 9, 2020 •

edited

Loading