Skip to content

Conversation

@mofterdinger
Copy link
Member

@mofterdinger mofterdinger commented Feb 11, 2025

  • Enhanced Books and Authors with details map
  • Provide initial data for the details fields in csv files

@mofterdinger mofterdinger changed the title Enhanced Books and Authors with details map. Showase type cds.map Feb 11, 2025
Copy link
Contributor

@agoerler agoerler left a 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 ;-)

@agoerler agoerler changed the title Showase type cds.map Showase type cds.Map Feb 11, 2025
@mofterdinger mofterdinger added the ☠️ do not merge ☠️ Do not merge PR label Feb 12, 2025
@mofterdinger mofterdinger removed the ☠️ do not merge ☠️ Do not merge PR label Feb 13, 2025
@agoerler agoerler merged commit 88ab48c into main Feb 27, 2025
2 checks passed
@agoerler agoerler deleted the show_cds_map branch February 27, 2025 21:27
@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");
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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?

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

Successfully merging this pull request may close these issues.

4 participants