refactor: Simplify Causality Lot stitching #3026

damassi · 2021-02-26T09:14:30Z

As I started working on moving the MyBids logic from Eigen to MP, I noticed some places where we might run into confusion/trouble later and wanted to open a PR up for discussion.

This refactors a previous PR to simplify our Causality Lot stitching code by deferring to more conventional stitching patterns seen throughout MP, removing indirection, and deleting a good amount of code.

All in all, no breaking changes have been made to the schema, and everything should be backwards compatible; however, as detailed below, we're now returning all watchedLotConnection nodes, versus just some. I'll defer to @erikdstock as to whether follow-up work is necessary in Eigen and am happy to tackle that as well.

{
  me {
    watchedLotConnection {
      edges {
        node {
          lot {
            id
          }
          saleArtwork {
            internalID
          }
        }
      }
    }
  }
}

Changes:

Previously, we created a new type Lot which handles lot state from Causality. In order to add a lot field to the Lot type (Lot being the node on a new WatchedLotConnection), we needed to make a request to causality by using a dataloader in our watchedLotConnection resolver, and then we added that response to the express req/res cycle context in order to pass it to our causality stitching logic, which is in a distant area of the codebase. This then extended the stitched causality AuctionLotState type with data fetched from causality via the causalityLoader, not from stitching.

The original assumption behind this was: how do we take a selection of all watched lots and then make one request to causality to get an array of lot state for all arworks, and then interleave the response in with the watched lots in a connection. This isn't exactly easy directly within stitched code. And the context solution was a clever one! But I think it might be a case of premature optimization in the sense that for given user who watches artworks, there's only going to be a small number of sales going on at any given time, and so we'd never be dealing with situation where suddenly the watched lots connection gets blown up with like 500 artworks. When we do get to that point and real world performance concerns are observed, we should solve it, but not until then. In meantime keep things as vanilla as we can.

And so this solution defers exclusively to conventional stitching techniques in that for n number of watched lots, we call causality's delegateToSchema and execute the lot(id: ...) query with the internalID of the artwork, which simplifies the feature substantially.

Additionally, I broke out the coupling between me.watchedLotConnection and Lot, since Lot is a more general type, where watchedLotConnection is a sub-field of me, and should be co-located there.

Another change is, previously if a sale artwork lacked lot state, it was excluded completely from the watchedLotConnection array, leading to confusion. A user could be watching x number of lots, but hasn't bid yet; those lots should still be represented in the connection ("a users watched lots") and for the purposes of the My Bids component, filtered at the UI level.

In the tests, indirection was also removed around testing MP-centric fields within causality's stitching code. Meaning: we were testing watchedLotsConnection far away from where watchedLotsConnection was defined.

--

In a follow-up PR I think some similar refactoring can be done around auctionsLotStandingConnection (based on discussions that Erik and I have been having), which will then put us in a good place to formally migrate the MyBids logic out of Eigen and into MP, to be shared with force.

damassi · 2021-02-26T09:20:08Z

src/lib/stitching/causality/__tests__/stitching.test.ts

    })
  })

-  describe("auctionsLotStandingConnection", () => {


All of this test code can be removed, because we no longer need to assert against what causality's stitched pass-through returns as we're now just making a simple query and attaching it to the Lot.lot field.

damassi · 2021-02-26T09:21:26Z

src/lib/stitching/causality/__tests__/stitching.test.ts

-      it("adds #auctionsLotStandingConnection to me", async () => {
-        const { getFields } = await useCausalityStitching()
-        expect(await getFields("Me")).toContain("auctionsLotStandingConnection")
+    it("resolves a SaleArtwork on an AuctionsLotStanding", async () => {


Most of this is just reorganizing to match the order of whats in the stitching file.

erikdstock · 2021-02-26T19:34:43Z

@artsy/purchase-devs cc & breadcrumbs

erikdstock

One question about a possible breaking change to the schema (field becomes nullable) but I am okay with the overall approach. Thanks for taking the time to understand and simplify this wacky code.

erikdstock · 2021-02-26T19:36:23Z

_schemaV2.graphql

 type Lot implements Node {
  # A globally unique ID.
  id: ID!

  # A type-specific ID likely used as a database ID.
  internalID: ID!
+  lot: AuctionsLotState


correct me if i'm wrong, but this is a breaking change, isn't it? Eigen clients will need to adapt to check lot state is present before rendering anything that requires it.

Yup, this is the follow-up I mentioned. The MyBids component will need to check if null in the loop, but its not a breaking schema change since the same query fields are still in place.

I think maybe we are talking about two slightly different different things. Isn't it possible this change would break eigen right here if a current app version loaded a lot with no lot state? https://github.com/artsy/eigen/blob/fcdad03ed67580286af2af27cf69a5eb71243c6e/src/lib/Scenes/MyBids/Components/WatchedLot.tsx#L18-L20

Ahh, I see what you mean. Here's a q. Outside of weird staging env, is it possible for there to be a lot with no lot state?

Almost certainly not!

erikdstock · 2021-02-26T19:44:19Z

_schemaV2.graphql

@@ -9006,6 +9004,9 @@ type Query {
    id: String!
  ): AuctionResult

+  # Returns a lot with specific `id`.
+  auctionsLot(id: ID!): AuctionsLotState


Not going to solve it in this PR but am reminded that we have a sticky situation building up with our naming conventions around 'lots':

SaleArtwork -> gravity's DB/model name for an auction lot

Lot -> Causality's name for an auction lot, lot id is same as gravity SaleArtwork id

lot/lotState -> two different names for the same thing in causality's graphql schema. I think we updated the schema to have lot be favored and lotState is deprecated, reasoning that state was redundant (the type is still LotState), but ...

Lot -> Metaphysics' name for the new, authoritative lot type that makes a causality lot field (of type AuctionsLotState) from causality available on the same object as sale artwork metadata. Thus it is possible to wind up with code that looks like lot.lot.reserveStatus which might be confusing.

Only saying this here to observe - if I had it to redo I would have kept the lotState version of the name.

Related to this comment on naming, I'm hesitant to introduce another new name to our schema.

Can this just be lot? Or is that reserved somewhere?

This is actually lot, but what we're doing is prefixing it with the stitched auctions prefix. Do you mean: don't do this?

(Also sorry, didn't see this comment before merging!)

Is this automagic? (Like how we append commerce to fields from exchange?) If so that's 👍 with me. If we were consciously making a decision here to add auctions- then I'd ask we don't add another name 😄.

Yup, it indeed is automagic ✨

erikdstock · 2021-02-26T19:46:52Z

src/lib/stitching/causality/schema.ts

@@ -10,7 +10,7 @@ import {
 import { readFileSync } from "fs"

 const blacklistedTypes: string[] = []
-const whitelistedRootFields: string[] = []
+const whitelistedRootFields: string[] = ["lot"]


similar comment here, if folks thought it was a good idea I would propose moving towards preferring fields named lotState when referring to the causality states. If there's no interest in this change I will get over it.

refactor: Simplify Causality lot stitching

55a1c15

damassi requested review from sepans and starsirius February 26, 2021 09:14

damassi assigned erikdstock Feb 26, 2021

damassi commented Feb 26, 2021

View reviewed changes

erikdstock reviewed Feb 26, 2021

View reviewed changes

damassi merged commit d7931e7 into master Mar 1, 2021

damassi mentioned this pull request Mar 1, 2021

feat: [WatchedLot] Guard against null artsy/eigen#4487

Merged

5 tasks

artsy-peril bot mentioned this pull request Mar 1, 2021

Deploy #3028

Merged

This comment was marked as off-topic.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Simplify Causality Lot stitching #3026

refactor: Simplify Causality Lot stitching #3026

damassi commented Feb 26, 2021 •

edited

Loading

damassi Feb 26, 2021 •

edited

Loading

damassi Feb 26, 2021

erikdstock commented Feb 26, 2021 •

edited

Loading

erikdstock left a comment

erikdstock Feb 26, 2021

damassi Mar 1, 2021

erikdstock Mar 1, 2021

damassi Mar 1, 2021

erikdstock Mar 1, 2021

erikdstock Feb 26, 2021

sweir27 Mar 1, 2021

damassi Mar 1, 2021 •

edited

Loading

sweir27 Mar 1, 2021

damassi Mar 1, 2021

erikdstock Feb 26, 2021

This comment was marked as off-topic.

refactor: Simplify Causality Lot stitching #3026

refactor: Simplify Causality Lot stitching #3026

Conversation

damassi commented Feb 26, 2021 • edited Loading

Changes:

damassi Feb 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikdstock commented Feb 26, 2021 • edited Loading

erikdstock 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damassi Mar 1, 2021 • edited Loading

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

This comment was marked as off-topic.

damassi commented Feb 26, 2021 •

edited

Loading

damassi Feb 26, 2021 •

edited

Loading

erikdstock commented Feb 26, 2021 •

edited

Loading

damassi Mar 1, 2021 •

edited

Loading