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

feat: adds sortable schema property [DHIS2-18749] #19612

Merged
merged 2 commits into from
Jan 9, 2025
Merged

feat: adds sortable schema property [DHIS2-18749] #19612

merged 2 commits into from
Jan 9, 2025

Conversation

jbee
Copy link
Contributor

@jbee jbee commented Jan 7, 2025

Summary

Adds a sortable property to schema Property that indicates if a property can be used in the order parameter.

By default only simple non-collections properties are sortable. Exceptions can be made by annotating the property getter with @Sortable(true) or @Sortable(false).

As far as I could tell we do not have any actual validation for the order parameter apart from simply ignoring collections and non-simple properties should they be present. Therefore that logic was used as the fall-back if no @Sortable annotation is present.

Cleanup

IDE supported refactoring to

  • use @Setter and some @Getter instead of explicitly stating the method
  • use final
  • add nullable annotations to overridden method

Manual changes

  • mark some computed properties as access = READ_ONLY for jackson

Manual Testing

check for example /api/schemas/dataElement and look at the properties array

Properties now have a sortable attribute, for example name looks like

{
      "klass": "java.lang.String",
      "propertyType": "TEXT",
      "name": "name",
      "fieldName": "name",
      "persisted": true,
      "description": "The name of this Object. Required and unique.",
      "attribute": true,
      "simple": true,
      "collection": false,
      "ordered": false,
      "owner": true,
      "identifiableObject": false,
      "nameableObject": false,
      "embeddedObject": false,
      "analyticalObject": false,
      "readable": true,
      "writable": true,
      "unique": true,
      "required": true,
      "length": 230,
      "max": 230.0,
      "min": 1.0,
      "manyToMany": false,
      "oneToOne": false,
      "manyToOne": false,
      "oneToMany": false,
      "translatable": true,
      "translationKey": "NAME",
      "i18nTranslationKey": "name",
      "gistPreferences": {
        "included": "AUTO",
        "transformation": "AUTO"
      },
      "sortable": true,
      "propertyTransformer": false
    }

@jbee jbee self-assigned this Jan 7, 2025
@jbee jbee requested a review from flaminic January 7, 2025 17:01

/** Object sharing (JSONB). */
protected Sharing sharing = new Sharing();
@Setter protected Sharing sharing = new Sharing();

Check notice

Code scanning / CodeQL

Missing Override annotation Note

This method overrides
IdentifiableObject.setSharing
; it is advisable to add an Override annotation.
@jbee jbee requested a review from a team January 7, 2025 17:54
Copy link
Contributor

@david-mackessy david-mackessy left a comment

Choose a reason for hiding this comment

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

can a test be added to check for the new field present?

@JacksonXmlProperty(namespace = DxfNamespaces.DXF_2_0)
public boolean isSortable() {
Sortable sortable = getterMethod == null ? null : getterMethod.getAnnotation(Sortable.class);
return sortable != null ? sortable.value() : !isCollection() && isSimple();
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 we need to check if the property is persisted , meaning it is mapped to a database column.
For example: a class may not have the property shortName but it is still inherit the property from base class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, but I will have to find a way to not exclude a computed property like displayName that way since it isn't persisted but still possible to sort by.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use persisted now where the default is that it has to be persisted. To make properties like displayName still be sortable they must be annotated with @Sortable(whenPersisted=false). This is not perfect but covers most of the cases.

@jbee
Copy link
Contributor Author

jbee commented Jan 9, 2025

can a test be added to check for the new field present?

Added a test now

Copy link

sonarqubecloud bot commented Jan 9, 2025

@jbee jbee requested a review from vietnguyen January 9, 2025 13:47
@jbee jbee merged commit 8654bf2 into master Jan 9, 2025
17 checks passed
@jbee jbee deleted the DHIS2-18749 branch January 9, 2025 16:11
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.

3 participants