-
Notifications
You must be signed in to change notification settings - Fork 160
Showase type cds.Map #439
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
Showase type cds.Map #439
Conversation
agoerler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Now we need to find out how what to actually do (display/modify) with the details ;-)
srv/src/main/java/my/bookshop/handlers/CatalogServiceHandler.java
Outdated
Show resolved
Hide resolved
| @After(event = CqnService.EVENT_READ) | ||
| public void bigBooks(Stream<Books> books) { | ||
| books.filter(b -> b.getDetails() != null).forEach(b -> { | ||
| int pages = (int) b.getDetails().get("pages"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if pages are not set, but null? This will crash the request. There is no guarantee that pages are set, especially for new books.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it with a new book and it worked, because details is already null and the filter catches this already. With the current bookshop I don't think it breaks here. Ok, we can add an additional null check for pages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I find this a strange concept here in general. Isn't the idea behind cds.Map that the data is generic and arbitrary and thus I can't really write business logic on it? From a modelling perspective I'd say a better practice would be to model pages explicitly. I get that this is a showcase of the feature, but I somehow fear that this example might lead to inappropriate usage. @agoerler what do you think?
Uh oh!
There was an error while loading. Please reload this page.