Skip to content

Database data getting added at wrong path #27

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

Closed
Venryx opened this issue Nov 30, 2017 · 7 comments
Closed

Database data getting added at wrong path #27

Venryx opened this issue Nov 30, 2017 · 7 comments

Comments

@Venryx
Copy link

Venryx commented Nov 30, 2017

The screenshot below should mostly explain it:

Basically, entries in the database are getting added at "users/USERID/USERID" -- instead of just "users/USERID". (ignore the correctly-added entry -- that is from a retrieval of the collection as a whole, which adds entries correctly without the extra wrapper)

I haven't yet determined what is causing the above issue to occur. It might be that I'm using the library incorrectly somewhere.

I will look into it, and find out what is causing the incorrect paths. Figured I would make an issue for it already, though, since the output above is still incorrect -- if from misuse, there should still probably be a catch/warning for it. (at the moment, it just happens silently)

@Venryx
Copy link
Author

Venryx commented Nov 30, 2017

It appears to occur by directly requesting the "users/USERID" document (instead of the collection as a whole):

store.firestore.setListener({
    collection: "versions",
    doc: "v6-dev",
    subcollections: [{collection: "users", doc: userID}]
});

Is this not the correct way to request the path "versions/v6-dev/users/USERID"?

This is the approach seemingly used here: #24 (comment)

@Venryx
Copy link
Author

Venryx commented Nov 30, 2017

At the moment, I am working around this issue with this terrible hack:

if (action.type == "@@reduxFirestore/LISTENER_RESPONSE") {
	if (action.payload.data) {
		// temp fix for "users/USERID/USERID" issue
		if (IsPathToDoc(action.meta)) { // if doc, remove extraneous wrapper around data
			action.payload.data = action.payload.ordered[0];
		}
	}
}

It runs as redux middleware, intercepting "LISTENER_RESPONSE" store-populator actions, and modifying the "data" property to hold the data for the requested document (eg. "users/USERID") directly, instead of in a wrapper. (eg. {USERID: data})

@prescottprue
Copy link
Owner

The v0.2.0 release includes the fix that @danleavitt0 PRed.

Let me know if it isn't working as expected. Thanks for the detailed breakdown of your issue and the patience while this library is still young.

@Venryx
Copy link
Author

Venryx commented Dec 12, 2017

On trying the latest version in my project, I found that it did not fix the problem for me -- in fact, it made it worse. (instead of having it in a wrapper, it now just places "undefined" at the location)

However, I looked into the code and found out how to resolve it.

First, lets look at the code in question: (lines 31-36 of src/reducers/dataReducer.js)

      const data = meta.doc ? get(payload.data, meta.doc) : payload.data;
      const previousData = get(state, pathFromMeta(meta));
      // Do not merge if no existing data or if meta contains subcollections
      if (!previousData || meta.subcollections) {
        return setWith(Object, pathFromMeta(meta), data, state);
      }

There are two problems with this code.

Problem 1

Consider this request meta:

{
    collection: "versions",
    doc: "v6-dev",
    subcollections: [{collection: "activities", doc: "1"}]
}

The problem is that, the code above is trying to "unwrap" the data using the meta's "doc" property; however, when referencing a subcollection the data is "wrapped" using the key for the subcollection/subdoc key, not the root doc key.

So we need to change the code to use the key for the actually-referenced (deep) sub-document (when referencing a subdocument), since that is what is used in the return-data wrapper.

Problem 2

Now consider this request meta:

{
    collection: "versions",
    doc: "v6-dev",
    subcollections: [{collection: "activities"}]
}

The problem here is similar to the first: trying to "unwrap" the data using the root "doc" key, even when we're not actually accessing a doc. (so there is no wrapper)

In this case, we want it to use payload.data directly as the data, instead of trying to unwrap it.

Fixed code

Just change line 31 from:

const data = meta.doc ? get(payload.data, meta.doc) : payload.data;

To:

const doc = meta.subcollections ? meta.subcollections.slice(-1)[0].doc : meta.doc;
const data = doc ? get(payload.data, doc) : payload.data;

I can confirm that the code fix above works in my project, resolving both issues encountered. I have not seen any negative side-effects so far -- though it's true that it's a pretty basic website, so it may not cover all use-cases.

@Venryx Venryx mentioned this issue Dec 12, 2017
@prescottprue
Copy link
Owner

@Venryx I noticed a worse condition for a few things to and started some fixes on this pull request for v0.2.1 (not sure if you saw those)

It is changed to:

const data = meta.doc && !meta.subcollections
  ? get(payload.data, meta.doc)
  : payload.data;

It does not contain the slice like your solution though, so going to add that.

@prescottprue prescottprue reopened this Dec 12, 2017
@prescottprue
Copy link
Owner

@Venryx Let me know if you get a chance to try it out. Want to make sure the fix correctly handles your use case

@Venryx
Copy link
Author

Venryx commented Dec 14, 2017

Tried out the latest version in the pull-request, and it does indeed fix both problems mentioned above.

@Venryx Venryx closed this as completed Dec 14, 2017
prescottprue added a commit that referenced this issue Dec 14, 2017
* fix(dataReducer): add exception for subcollections in already existing path (relating to code added by #31 )
* fix(dataReducer): data written to correct path through improved support for data unwrapping - #27 - @Venryx
prescottprue added a commit that referenced this issue Dec 14, 2017
* fix(dataReducer): add exception for subcollections in already existing path (relating to code added by #31 )
* fix(dataReducer): data written to correct path through improved support for data unwrapping - #27 - @Venryx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants