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

Could you update me about what is left for this project to be complete? #16

Open
joseincandenza opened this issue Oct 7, 2019 · 3 comments

Comments

@joseincandenza
Copy link

Hi,

@fairingrey apart from tests, could you briefly expose what is this project missing to be published as a complete realworld-example-app?

Thank you.

@joseincandenza
Copy link
Author

joseincandenza commented Oct 9, 2019

@fairingrey

I'm currently studying your code, using it as a base for another app. I have experience using rust, but never used it to build a server, so my experience with diesel and actix is None.

I would like to help you to push this project to completion, if you are interested. I have some doubts, though, probably derived from the mentioned lack of experience.

For example, I don't understand the query logic that you implement when obtaining the DbExecutor Handle of GetArticles. Here you apply conditional logic to obtain the list filtered by the params pased by the user, but you make multiple calls to the database, and I don't get why:

if let Some(ref author_name) = msg.params.author {
	let articles_by_author = articles::table
		.inner_join(users::table)
		.filter(users::username.eq(author_name))
		.select(articles::id)
		.load::<Uuid>(conn)?;

		query = query.filter(articles::id.eq_any(articles_by_author));
        }

Won't be easier/cheaper to just extend the query without touching the database? According to documentation, filter is comulative, so you can just chain filters conditionally:

if let Some(ref author_name) = msg.params.author {
	query = query.filter(users::username.eq(author_name))
}

The query would be previously joined. And executed after all the possible filters.

Another point is the use of eq_any. According to the documentation, with postgres you should use eq(any()).

I have not yet started with tests, so I cannot know if this will work, but I thought you probably know better.

Thank you in advance.

@fairingrey
Copy link
Owner

Hey, sorry I haven't replied to this post in a while, since I've been busy with work and haven't found the time to do much open-source. Also the time I do get I focus more on Tide and other frameworks with async/await as first-class syntax. Especially as my work involves those things directly.

Yeah, throughout the code I've been meaning to clean up quite a few places where I'm inefficiently executing multiple queries through diesel. The explanations you're giving make sense, although I can't verify at the moment whether they will actually compile (and I have yet to re-review the original scheme for each of these tables).

Would you like to make those changes? I'd be happy to review and accept a PR with them.

@fairingrey
Copy link
Owner

As for what this project is missing, you are welcome to explore yourself what else this project requires or bump its' corresponding issue on the original realworld repo.

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