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

Add support for embedded objects #565

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

marcosmko
Copy link

@marcosmko marcosmko commented May 19, 2021

Implementation for @Embed annotation based on #328 by @hsul4n.

In this approach, you declare an Embed class exactly like you declare an Entity, with support for ColumnInfo annotation too.

@Embed()
class Timestamp {
  @ColumnInfo(name: 'created_at')
  final DateTime createdAt;

  @ColumnInfo(name: 'updated_at')
  final DateTime updatedAt;

  Timestamp({required this.createdAt, required this.updatedAt});
}

When declaring your FloorDatabase, you pass to @Database annotation the classes that are for embedding in other entities (I also added TypeConverter for DateTime).

@TypeConverters([DateTimeConverter])
@Database(version: 1, entities: [Task], embeds: [Timestamp])
abstract class FlutterDatabase extends FloorDatabase {
  TaskDao get taskDao;
}

And finally, you add the Embed type to your Entity.

@Entity
class  Task {
  @PrimaryKey(autoGenerate: true)
  final int? id;

  final String message;

  // @ColumnInfo(name: '')
  final Timestamp timestamp;

  Task(this.id, this.message, this.timestamp);
}

Note that I added the possibility for naming the embed type. If you do not declare a name, it will use the field name as a prefix. If you declare an empty name, then it will embed the Timestamp fields directly.

For example, in the provided example it will generate timestamp_created_at and timestamp_updated_at columns, while if you provide an empty name it will generate created_at and updated_at columns.

Closing #9

@marcosmko
Copy link
Author

Hello @vitusortner and @mqus , I re-made this feature using a better approach for declaring embedded classes.

There are still some problems like creating the column name, but I would like to request a first review for validating this approach.

@marcosmko
Copy link
Author

@vitusortner Updated PR description explaining how to use this feature.

}
return parameterValue; // also covers positional parameter
} else if (field is Embed) {
// return _getConstructor(field.classElement, [...field.fields, ...field.children]);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete

);

/// The database column definition.
String getDatabaseDefinition(final bool autoGenerate) {
if (embedConverter != null) {
throw InvalidGenerationSourceError(
'You ',
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch message error

Copy link
Collaborator

@mqus mqus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking the time to implement this!

I really like that you take the road of the type converters, this seems to fit in well, as type converters basically do the same thing (modulo converting one to many attributes and being able to set converters more precise). But for the API surface I rather like rooms approach more (see the comments). This is just a rough review though and @vitusortner surely also has some opinions :D

I enabled the CI pipeline, just ask if you want to know something :)

I'm curious to hear your opinions.

import 'package:floor/floor.dart';
import 'package:sqflite/sqflite.dart' as sqflite;

part 'database.g.dart';

@Database(version: 1, entities: [Task])
@TypeConverters([DateTimeConverter])
@Database(version: 1, entities: [Task], embeds: [Timestamp])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to include the embeds here? doesn't it suffice to annotate the usage in the entities/views?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to include if we annotate the embedded class, if we annotate the variable like room then we don't need to include here.

@@ -7,7 +8,10 @@ class Task {

final String message;

Task(this.id, this.message);
// @ColumnInfo(name: '')
final Timestamp timestamp;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm personally in favor of having @embedded here, like in room to be able to more finely adjust where to embed and where to type convert... what are your opinions here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello sir!

I don't have a background in Android/Java so I was not aware of the room approach.

I think that annotating the class as an embedded type is easier as we need to annotate it just once, instead of annotating every time we declare it inside an entity class. For finely adjusting, as we have just the prefix string, I thought we could use the ColumnInfo name as we already do for other variables.

If you prefer the room approach I can modify it, just tell me :)

@marcosmko
Copy link
Author

Hello, any news here?

@Johann673
Copy link

Very nice feature, thank you @marcosmko for the job !

I have one suggestion :
It can be very nice if the Emeded object in entity can be nullable.

For exemple :

@entity
class Crew {
  @primaryKey
  final int id;

  @ColumnInfo(name: 'employee')
  final Employee? employee; // Employee can be null, so "employee_id" is nullable and also "employee_name"

  Crew({
    required this.id,
    this.employee
  });
}

@embed
class Employee {
  final int id;
  final String name;

  Employee({required this.id, required this.name});
}

Thanks you !

@marcosmko
Copy link
Author

Hello @Johann673 , actually I'm still waiting for the project owner to review this PR so I could make improvements... 😅

@DesmondFox
Copy link

Looks great but why still don't reviewed? 🤔

@mataide
Copy link

mataide commented Nov 14, 2021

What is missing?

@mataide
Copy link

mataide commented Nov 14, 2021

@marcosmko An embed class can't be an Entity also?

@DesmondFox
Copy link

Why is it not merged? I just need this feature

@marcosmko
Copy link
Author

Waiting for code review, guys :/

@DesmondFox
Copy link

a lot of time has passed. Didn't the owner have time to review?
@vitusortner

@Kraigo
Copy link

Kraigo commented Feb 7, 2023

What's progress for this feature?

@dkaera
Copy link
Collaborator

dkaera commented Feb 7, 2023

What's progress for this feature?

Currently it postponed for a while. The MR is so far from the actual develop, so it needs to review again and update.

@Velkamhell97
Copy link

Any update ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants