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

fix #3620: throw a meaningful exception if no kind/plural, default plural #3624

Merged
merged 2 commits into from
Dec 9, 2021

Conversation

shawkins
Copy link
Contributor

@shawkins shawkins commented Nov 29, 2021

Description

fix #3620: throw a meaningful exception if no kind/plural, and default plural based upon kind.

It looks like the code is tolerant to the other fields being missing - including group.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • I Added CHANGELOG entry regarding this change
  • I have implemented unit tests to cover my changes
  • I have added/updated the javadocs and other documentation accordingly
  • No new bugs, code smells, etc. in SonarCloud report
  • I tested my code in Kubernetes
  • I tested my code in OpenShift

@centos-ci
Copy link

Can one of the admins verify this patch?

@shawkins shawkins changed the title fix #3620: throw a meaningful exception if no kind, default plural fix #3620: throw a meaningful exception if no kind/plural, default plural Nov 29, 2021
@shawkins
Copy link
Contributor Author

kind is not strictly needed. updated the pr to relax that.

@rohanKanojia
Copy link
Member

rohanKanojia commented Nov 30, 2021

Sonar is complaining about the use of @Deprecated method Utils.getPluralFromKind . Is it possible to use something else for calculating plural here? Maybe use Pluralize.getPlural directly...

@shawkins
Copy link
Contributor Author

Is it possible to use something else for calculating plural here?

I'm a fan of not duplicating code - I know this is somewhat trivial, but anything else would simply be a copy of that.

if (kind == null) {
throw new IllegalArgumentException("Neither kind nor plural was set, at least one is required");
}
plural = Utils.getPluralFromKind(kind);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This part is tricky because the target class might use a different plural version than the one computed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the context is constructed from a class it will use the annotation to populate this field. This will only occur in case where someone is manually constructing the context. Every method the specifies both a context and a class has been deprecated - it's not expected that a user will manually construct the context when a class is used.

@manusa
Copy link
Member

manusa commented Nov 30, 2021

When Alex reported that issue, I was thinking more of an Objects.requireNonNull wherever the plural was consumed to compute the URL. However, I haven't analyzed and maybe it's a better approach to validate the definition context, or both.

@shawkins
Copy link
Contributor Author

However, I haven't analyzed and maybe it's a better approach to validate the definition context, or both.

Since these contexts are the only place that the plural can come from and are immutable it's sufficient to validate them as they are created. That will also provide the most meaningful feedback.

@manusa manusa requested a review from metacosm December 2, 2021 10:37
@manusa manusa added this to the 5.11.0 milestone Dec 2, 2021
…sl/base/ResourceDefinitionContextTest.java

Co-authored-by: Chris Laprun <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Dec 9, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@manusa manusa merged commit bd602b8 into fabric8io:master Dec 9, 2021
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.

Throw a meanful exception error instead of NPE when no plurals are set
6 participants