Skip to content
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

How to create a new document with a reference? #118

Open
lucasdidur opened this issue Nov 12, 2019 · 18 comments · May be fixed by #238
Open

How to create a new document with a reference? #118

lucasdidur opened this issue Nov 12, 2019 · 18 comments · May be fixed by #238
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@lucasdidur
Copy link

Hello, how can I create a new ProdutorDadosAdicionais with a referencied property produtor of Produtor class?

@Collection()
export class Produtor {
    id: string
    nome?: string;
}

@Collection()
export class ProdutorDadosAdicionais {
    id: string;
    nome: string;
    produtor: Produtor;
}

I tried this, but no gave me a error:

  | Error: Value for argument "data" is not a valid Firestore document. Couldn't serialize object of type "Produtor" (found in field produtor). Firestore doesn't support JavaScript objects with custom prototypes (i.e. objects that were created via the "new" operator). -- | --   | at Object.validateUserInput (C:\Users\lucas.didur\Desktop\Nova pasta\functions\node_modules\@Google-Cloud\firestore\build\src\serializer.js:311:15)   | at validateDocumentData (C:\Users\lucas.didur\Desktop\Nova pasta\functions\node_modules\@Google-Cloud\firestore\build\src\write-batch.js:611:22)   | at WriteBatch.set (C:\Users\lucas.didur\Desktop\Nova pasta\functions\node_modules\@Google-Cloud\firestore\build\src\write-batch.js:232:9)   | at DocumentReference.set (C:\Users\lucas.didur\Desktop\Nova pasta\functions\node_modules\@Google-Cloud\firestore\build\src\reference.js:334:14)   | at BaseFirestoreRepository. (C:\Users\lucas.didur\Desktop\Nova pasta\functions\node_modules\fireorm\lib\src\BaseFirestoreRepository.js:109:50)   | at step (C:\Users\lucas.didur\Desktop\Nova pasta\functions\node_modules\fireorm\lib\src\BaseFirestoreRepository.js:46:23)   | at Object.next (C:\Users\lucas.didur\Desktop\Nova pasta\functions\node_modules\fireorm\lib\src\BaseFirestoreRepository.js:27:53)   | at fulfilled (C:\Users\lucas.didur\Desktop\Nova pasta\functions\node_modules\fireorm\lib\src\BaseFirestoreRepository.js:18:58)   | at   | at process._tickCallback (internal/process/next_tick.js:189:7)
let doc = new ProdutorDadosAdicionais();
doc.nome = "Teste";
doc.produtor = await prod.findById("ITXaj7Gss5dt1cOpT0qf");

await repo.create(doc);
@wovalle
Copy link
Owner

wovalle commented Nov 14, 2019

I understand the problem but what is what you're trying to achieve? If you want Produtor to be a subcollection of ProdutorDadosAdicionais, you should remove the @Collection decorator from Produtor and follow the subcollection example.

If what you want is to stablish a 1:1 relationship between your entities I would start removing the @Collection decorator.

Apart from that, I see that there might be a bug with the deserialization/serialization of the firestore entities when the values are class instances. I could recommend adding produtorId and produtorNome as properties in ProdutorDadosAdicionais while this is fixed

@wovalle wovalle added bug Something isn't working help wanted Extra attention is needed labels Nov 14, 2019
@nietzscheson
Copy link

nietzscheson commented Jan 27, 2020

@wovalle I think that @lucasdidur is doing reference to: https://firebase.google.com/docs/firestore/data-model#references for me it's more easy can related documents with references that as subcollections.

@Kronhyx
Copy link
Contributor

Kronhyx commented Feb 18, 2020

@lucasdidur @wovalle @nietzscheson
Have you been able to find any solution for this?

@nietzscheson
Copy link

@Kronhyx I found this similar ORM

@wovalle
Copy link
Owner

wovalle commented Feb 25, 2020

Thanks! Didn't know about firestorm (and I'm mad because that name is awesome :D). I updated my roadmap (#1) to increase the compatibility with raw firestore documents. For now I'll close this issue.

@wovalle wovalle closed this as completed Feb 25, 2020
@LuisDev99
Copy link

Would you like to consider re-opening this issue @wovalle ?

@wovalle wovalle reopened this Feb 20, 2021
@wovalle
Copy link
Owner

wovalle commented Feb 20, 2021

Here's some documentation about searching by reference: https://fireorm.js.org/#/Read_Data?id=search-by-document-reference

What other do you guys think are missing?

@LuisDev99
Copy link

LuisDev99 commented Feb 20, 2021

@wovalle thank you!

It would be really great if we could this:

@Collection()
class Artist { 
  id: string;
  name: string;
  // some other useful props
}

@Collection()
class Band {
  id: string;
  
  @DocumentReference()
  artist: Artist;
}

Then this:

getRepository(Band).create( { 
    id: 'fireorm rocks!',
    artist: '/artists/wovalle'   // Making a reference to a Collection ```artist``` to a document ```wovalle```
});

That way, when we create a new band document, artist will appear as a document reference data type in firestore, giving us the opportunity to do this:

getRepository(Band).find().forEach((band) => console.log(band.artist.name));

As you can see, it is really easy to access the artist of a particular band without having the need to go fetch the artist or making a subcollection. Let me know what you think and I will be happy to help you

@wovalle
Copy link
Owner

wovalle commented Feb 22, 2021

That API looks nice indeed!

Some questions that are still unanswered:

In your example artist was declared as Artist but in the create, artist is being set by the reference path (string). What if we pass an Artist object instead? It needs to be an Artist in the model declaration to allow typescript completion.

It would look like this:

const artist = new Artist()
artist.id = 'new-artist'
artist.name = "New Artist"

getRepository(Band).create({ 
   id: 'fireorm rocks!',
   artist,
});

What would happen if you try to save an Album with a new artist (aka still not saved in firestore)?

  • Should fireorm do it for you (inside a transaction)?
  • Should fireorm throw an error
  • Should fireorm provide a helper function like setReference and then determine if it should be created or not?
const artist = new Artist()
artist.id = 'new-artist'
artist.name = "New Artist"

getRepository(Band).create({ 
   id: 'fireorm rocks!',
   artist: setReference(artist),
});

@LuisDev99
Copy link

In your example artist was declared as Artist but in the create, artist is being set by the reference path (string). What if we pass an Artist object instead? It needs to be an Artist in the model declaration to allow typescript completion.

Yes, I set the artist in the create method as a string. By adding the @DocumentReference() decorator, the property Artist can be assign either an object or a string that points to its desired reference (I forgot to put Artist | string, read below to know the reason)

What would happen if you try to save an Album with a new artist (aka still not saved in firestore)?

Should fireorm do it for you (inside a transaction)?
Should fireorm throw an error
Should fireorm provide a helper function like setReference and then determine if it should be created or not?

I believe that fireorm should just let you or throw an error at best given that firestore allows you to set a reference to a non-existent document when the reference is of type string. It should be the developer's job to ensure he's doing a valid reference IMO.

The reason I assigned the artist a string inside the create method

getRepository(Band).create( { 
    id: 'fireorm rocks!',
    artist: '/artists/wovalle'   // Making a reference to a Collection ```artist``` to a document ```wovalle```
});

is to be able to check to see if the artist property is of type string or object.

  • If it's of type string, we let the create method do the insert normally and nothing more
  • If it's of type object, it would be nice to create the Artist first and then, assign the reference to the newly created artist inside a single transaction.

This way, we let the create method accept both of this options. For example, like this:

/* Create example with a string */ 
 getRepository(Band).create({ 
    id: 'fireorm rocks!',
    artist: '/artists/wovalle'   // I'm a string, the create method should do the insert normally! 
                                 // (And I'll still be able to appear as a firestore document reference data type)
});
 
/* Create example with an object */
 getRepository(Band).create({ 
    id: 'fireorm rocks!',
    artist: { 
       id: 'new artist from a newly created band',
       name: 'cisco'
    }      // In here, I'm an artist object, maybe fireorm can create me first and then create the band and 
           // automatically reference me! (And still be able to appear as a firestore document reference data type)
});

Let me know what you think 🙏🏻

@wovalle
Copy link
Owner

wovalle commented Feb 23, 2021

Instead of forcing the user to declare all of the references as T | string it'll be a better idea to provide a generic type that accepts T and resolves to T | string (something like: Reference<Artist>)

@Collection()
class Band {
  id: string;
  
  @DocumentReference()
  artist: Reference<Artist>;
}

Given that the reference field must be decorated with DocumentReference, in runtime we save it if is T or just save the reference if is string. Is there a way to validate that the reference string is a valid path?

And one problem I find here is that we're using plain objects as the parameter for create where fireorm receives T. I guess something has to be done to receive T or something that looks like T (A extends T?)

Another problem is that as of today, there's no easy way to get the path (ref?) of a fireorm entity. To achive this something must be done. Maybe adding a getRef/getPath function in each entity might solve this. What would be better? Is ref always the path to the document? 🤔

@LuisDev99
Copy link

@DocumentReference()
  artist: Reference<Artist>;

Looks really great! Couldn't agree more.

Is there a way to validate that the reference string is a valid path?

This is what the Google API NodeJS Firestore repository does to validate a path:

export function validateResourcePath(
  arg: string | number,
  resourcePath: string
): void {
  if (typeof resourcePath !== 'string' || resourcePath === '') {
    throw new Error(
      `${invalidArgumentMessage(
        arg,
        'resource path'
      )} Path must be a non-empty string.`
    );
  }

  if (resourcePath.indexOf('//') >= 0) {
    throw new Error(
      `${invalidArgumentMessage(
        arg,
        'resource path'
      )} Paths must not contain //.`
    );
  }

They just check if the path is a valid string and does not contains any double slash.

Another problem is that as of today, there's no easy way to get the path (ref?) of a fireorm entity. To achive this something must be done. Maybe adding a getRef/getPath function in each entity might solve this. What would be better?

The getPath sounds great! I guess the implementation would go something like this:

return this.className(?) + '/' + this.id; // I actually don't know how you take the classname (or the collection name if overriden) of the entity lol

Is ref always the path to the document?

Yes!

This link might be helpful as well

@wovalle
Copy link
Owner

wovalle commented Feb 28, 2021

About the validation: That's easy then 🙌 , In the future I imagine adding an option to prevent adding a relationship that doesn't exist yet.

About getPath: every repository has access to the path this.path. Whenever fireorm constructs a class (via creating an object or doing a query) extractTFromDocSnap is called, so to append the path to each document would be as easy as passing the path there (with a readonly variable name close to fireorm_internal_path and adding the getPath method in the abstract repository that will return it.

@wovalle wovalle linked a pull request Feb 28, 2021 that will close this issue
@wovalle
Copy link
Owner

wovalle commented Feb 28, 2021

Did the initial work to support references in #238! Still a long way to go though!

Check this test

I found out that my initial Reference<Artist> suggestion won't work. I'd like to be able to do something like band.reference.id which is not possible if reference is declared as Reference<Artist>.

@LuisDev99
Copy link

Awesome!

expect((savedSW.relatedBand as Band).id).toEqual(nm.id);

You mean from this right (the casting)? I really don't know a clean solution for that

@wovalle
Copy link
Owner

wovalle commented Mar 2, 2021

Yeah I'll have to rethink the api. The only thing that occurs to me at the moment is to leave it as T and add a second parameter to create to pass the paths, but that'll be more challenging.

Something like:

/* Create example with an object */
getRepository(Band).create({
  id: 'fireorm rocks!',
  artist: {
    id: 'new artist from a newly created band',
    name: 'cisco',
  },
});

getRepository(Band).create(
  {
    id: 'fireorm rocks!',
  },
  { refs: { artist: '/artists/wovalle' } }
);

@LuisDev99
Copy link

LuisDev99 commented Mar 3, 2021

It could be an alternative. Right now, I'm looking at what Firebase Firestorm does to create a document reference when creating a new document.

They have this:

export default class Post extends Entity {
  @documentRef({
    name: 'author',
    entity: Author, // we must define the entity class due to limitations in Typescript's reflection capabilities. Progress should be made on this issue in future releases.
  })
  author!: IDocumentRef<Author>;
  ...
}

So I'll look if they have a way of creating a document reference inside the create method. If they do, I'll check out the code and share it with you to give more ideas

@Kronhyx
Copy link
Contributor

Kronhyx commented Jul 30, 2021

any update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants