Skip to content

Issue 51979: BadSqlGrammarException indexing sample types immediately after a field rename#6611

Merged
XingY merged 5 commits intodevelopfrom
fb_issue51979
May 2, 2025
Merged

Issue 51979: BadSqlGrammarException indexing sample types immediately after a field rename#6611
XingY merged 5 commits intodevelopfrom
fb_issue51979

Conversation

@XingY
Copy link
Copy Markdown
Contributor

@XingY XingY commented Apr 28, 2025

Rationale

Issue 51979: BadSqlGrammarException indexing sample types immediately after a field rename

  • Switch to use TableSelector instead of SqlSelector for material index document preparation.
  • Refactor material and exp data to share index util

Issue 52961: DataClass: Integer fields are not index for data class, unlike sample types

  • add integer field type to index document

Related Pull Requests

Changes

@XingY XingY self-assigned this Apr 28, 2025
/** returns null if the parent container is no longer available */
@Nullable
public WebdavResource createIndexDocument()
public WebdavResource createIndexDocument(ExpMaterialTableImpl tableInfo)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mark as @Nullable ExpMaterialTableImpl tableInfo

});
}

public void processIndexValues(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider making this protected.

tableInfo = (ExpMaterialTableImpl) QueryService.get().getUserSchema(User.getSearchUser(), container, SCHEMA_SAMPLES).getTable(getSampleType().getName());

if (tableInfo != null)
getCustomIndexValues(props, tableInfo, jsonData);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be passing through the maps like ExpDataImpl does?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. Updated to pass in identifiersHi map. The other maps are not needed at the moment.

Copy link
Copy Markdown
Member

@labkey-tchad labkey-tchad 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 from my manual testing.
One thing I noticed though; field renames aren't included in a sample's audit timeline. Maybe they should be?

@labkey-susanh
Copy link
Copy Markdown
Contributor

Looks good from my manual testing. One thing I noticed though; field renames aren't included in a sample's audit timeline. Maybe they should be?

Field renames are included in the Domain Property Events, which are, indeed, not pulled into the sample's timeline.

Please open a separate issue and assign to triage to assess whether we need to do anything additional about this auditing.

# Conflicts:
#	experiment/src/org/labkey/experiment/api/AbstractRunItemImpl.java
@XingY XingY merged commit 6d8a776 into develop May 2, 2025
1 of 16 checks passed
@XingY XingY deleted the fb_issue51979 branch May 2, 2025 02:19
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