Ensure Auth entity has a unique username #1257

sodic · 2023-06-15T20:24:42Z

I couldn't find any JS code that deals with this: #769 (comment)

sodic · 2023-06-15T20:30:46Z

waspc/src/Wasp/AppSpec/Entity.hs

+doesFieldHaveAttribute :: String -> String -> Entity -> Maybe Bool
+doesFieldHaveAttribute fieldName attrName entity =
+  doesPslFieldHaveAttribute attrName <$> findPslFieldByName fieldName entity
+
+findPslFieldByName :: String -> Entity -> Maybe PslModel.Field
+findPslFieldByName fieldName Entity {pslModelBody = PslModel.Body elements} =
+  find isField [field | (PslModel.ElementField field) <- elements]
+  where
+    isField PslModel.Field {_name = name} = name == fieldName
+


@infomiho @Martinsos

I wasn't sure how to decide what to put in here vs. in Psl/Util.hs.

This is the only file that uses Psl/Util.hs. Should we maybe remove Psl/Util.hs (especially considering the conversation about keeping PslModel stuff hidden and working only with regular Fields)?

Finally, if we do decide to hide everything related to PslModel, should I refactor the getIdField and getIdBlockAttribute functions to make them fit into that.

I am missing a bit of context so it is somewhat hard for me to make a decisive statement here, but:

If we have functions operating on PSL stuff and not knowing anything about Entity, then it sounds good to have them in PslUtil.hs, regardgless by how many other modules are using them.

For future, it might help if you give more context. For exmaple, what is in Psl/Util.hs, why do you think it shouldn't be there (besides this being the only place using it), what are pros/cons of each approach (aka "what have you tried"), what do getIdField and getIdBlockAttribute do to make them candidates for this refactoring, how much work is that, what are the benefits, ... .

Also, I like the idea of exposing Entity + ENtity.Filed interface and hiding PSL if possible. If not possible / practical, we can let some PSL seep out.
I can't judge right now how possible / practical it is without diving deeper, but I believe you will know that better!

I liked the split of having Entity using helper functions from Psl/Util.hs.

Example of such fn:

getIdField :: Entity -> Maybe PslModel.Field getIdField = findIdField . getPslModelBody

All the low level work on the PSL schema is hidden away in the PslUtil module, while Entity is composing those function to get out what they need without worrying about the implementation.

For this PR, I think the split could go like this:

Psl/Util.hs implements functions for checking if certain field in PSL has some attribute attached isAttributePresentOnField

Entity.hs uses that function to come up with a function isFieldUnique (looking for @unique)

Valid.hs can then do something like Entity.isFieldUnique "username"

The main idea was as follows:

If we're hiding PSL as an implementation detail (this is the core assumption), there probably aren't many reasons for having a Utils file (in its current location).

The only module that should ever be using those utils is Entity (since all PSL stuff is a "hidden" implementation detail), so we might as well move all those functions there (instead of having them globally available from a different part of the codebase).

Such an organisation colocates all relevant code and discourages/prevents us from acting on PSL directly (i.e., without going through the functions exposed by the Entity module). The low-level PSL stuff would still be abstracted away by appropriate functions that aren't exported from Entity.

I've added the isFieldUnique function and left the other stuff as is for now.

@sodic not so important for this PR, but I would do the opposite -> I would put PSL specific stuff in PSL/Utils.hs or something like that, not in Entity, IF it makes sense to be standalone. This is useful because it clearly indicates that this code is not Entity specific, in the sense that there are some assumptions they share. Entity would still be having that as an implementation details, that doesn't change. And the fact that nobody else is using it is also not an issue, if it is general it is general -> we might need it in some other parts of the code, even if also hidden as an implementation detail.
Otherwise we could just move the whole PSL module under Entity. But I like it better that we say "hey, look we have PSL module because Wasp knows how to work with it!" without assuming who is using it.

But this is not a biggie, we can in any case move it around if needed.

sodic · 2023-06-15T20:38:00Z

@Martinsos What did you mean by this comment: #769 (comment). Should I also remove some JS code?

Martinsos · 2023-06-16T07:45:54Z

@Martinsos What did you mean by this comment: #769 (comment). Should I also remove some JS code?

Unfortunately I can't remember, I wish I wrote more info. I guess we, when creating user via #, check there, in the Prisma request, if username is unique? But we don't enforce it in the schema itself?

infomiho · 2023-06-16T09:11:39Z

waspc/test/AppSpec/EntityTest.hs

@@ -8,10 +8,17 @@ import qualified Wasp.Psl.Ast.Model as PslModel
 spec_AppSpecEntityTest :: Spec
 spec_AppSpecEntityTest = do
  describe "getIdField" $ do
-    it "gets primary field from entity when it exists" $ do


We have 19 test files where we write the first letter as upper case and 21 files where we write it as lower case.

Obviously we have two competing standards 😄 @sodic @Martinsos

I vote uppercase, same goes for git messages 😄

They're nicer to read.

Ha I also wasn't sure, but sounds good it if is nicer to read! Git messages yes, for sure, upper letter and dot at the end!

infomiho · 2023-06-16T09:21:36Z

waspc/src/Wasp/AppSpec/Valid.hs

+        | doesFieldHaveAttribute "username" "unique" userEntity == Just True = []
+        | otherwise =
+          [ GenericValidationError $
+              "The field 'username' on Entity referenced by " ++ userEntityName ++ " must have the '@unique' attribute."


I would go for less technical error message like

The username field on the User entity should be marked as unique (add the @unique attribute on the username field).

If some new users this super technical message, it might confuse them IMHO

I agree, but the rest of the file uses this style so I decided against changing it to save time. I can make an issue for refactoring the error messages.

Consider changing this message and the others (I see only one other:

wasp/waspc/src/Wasp/AppSpec/Valid.hs

Line 245 in afe90a2

"Expected an Entity referenced by " ++ entityName ++ " to have field '" ++ fieldName ++ "' of type '" ++ fieldTypeName ++ "'."

) if you have the time 👍

infomiho · 2023-06-16T09:22:51Z

Tested it out locally, it works. Some minor comments regard the error message and code organisation.

…ername

sodic added 2 commits June 15, 2023 19:17

Ensure unique username on app.auth.userEntity

92639d5

Update tests

ff73661

sodic commented Jun 15, 2023

View reviewed changes

Rearrange code to read like a story

afe90a2

infomiho reviewed Jun 16, 2023

View reviewed changes

sodic added 5 commits June 16, 2023 14:39

Improve abstraction

603f9cf

Merge branch 'main' of github.com:wasp-lang/wasp into filip-unique-us…

a6cd169

…ername

Improve error messages

54e0bac

Further improve error message

b2baad5

Fix formatting

1c687e5

sodic merged commit 90bbff3 into main Jun 16, 2023

sodic deleted the filip-unique-username branch June 16, 2023 15:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure Auth entity has a unique username #1257

Ensure Auth entity has a unique username #1257

sodic commented Jun 15, 2023 •

edited

Loading

sodic Jun 15, 2023 •

edited

Loading

Martinsos Jun 16, 2023 •

edited

Loading

Martinsos Jun 16, 2023

infomiho Jun 16, 2023

sodic Jun 16, 2023 •

edited

Loading

sodic Jun 16, 2023

Martinsos Jun 19, 2023

sodic commented Jun 15, 2023

Martinsos commented Jun 16, 2023

infomiho Jun 16, 2023

sodic Jun 16, 2023

Martinsos Jun 19, 2023

infomiho Jun 16, 2023

sodic Jun 16, 2023 •

edited

Loading

infomiho Jun 16, 2023

infomiho commented Jun 16, 2023

Ensure Auth entity has a unique username #1257

Ensure Auth entity has a unique username #1257

Conversation

sodic commented Jun 15, 2023 • edited Loading

sodic Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

Martinsos Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sodic Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sodic commented Jun 15, 2023

Martinsos commented Jun 16, 2023

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

sodic Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infomiho commented Jun 16, 2023

sodic commented Jun 15, 2023 •

edited

Loading

sodic Jun 15, 2023 •

edited

Loading

Martinsos Jun 16, 2023 •

edited

Loading

sodic Jun 16, 2023 •

edited

Loading

sodic Jun 16, 2023 •

edited

Loading