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 inconsistent search results for date-related queries fixes#12296 #12724

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv
- We fixed an issue where JabRef displayed an incorrect deletion notification when canceling entry deletion [#12645](https://github.com/JabRef/jabref/issues/12645)
- We fixed an issue where JabRef displayed an incorrect deletion notification when canceling entry deletion. [#12645](https://github.com/JabRef/jabref/issues/12645)
- We fixed an issue where an exception would occur when running abbreviate journals for multiple entries. [#12634](https://github.com/JabRef/jabref/issues/12634)
- We fixed an issue Where JabRef displayed an inconsistent search results for date-related queries[#12296](https://github.com/JabRef/jabref/issues/12296)

### Removed

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
import java.sql.SQLException;
import java.util.Arrays;
import java.util.Collection;
import java.util.HashSet;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.regex.Pattern;

import org.jabref.logic.l10n.Localization;
Expand Down Expand Up @@ -38,6 +40,7 @@ public class BibFieldsIndexer {
private static final Logger LOGGER = LoggerFactory.getLogger(BibFieldsIndexer.class);
private static final LatexToUnicodeFormatter LATEX_TO_UNICODE_FORMATTER = new LatexToUnicodeFormatter();
private static final Pattern GROUPS_SEPARATOR_REGEX = Pattern.compile("\s*,\s*");
private static final Set<Field> DATE_FIELDS = new HashSet<>(Arrays.asList(StandardField.DATE, StandardField.YEAR, StandardField.MONTH, StandardField.DAY));

private final BibDatabaseContext databaseContext;
private final Connection connection;
Expand Down Expand Up @@ -215,10 +218,12 @@ private void addToIndex(BibEntry bibEntry) {
// We add a `.orElse("")` only because there could be some flaw in the future in the code - and we want to have search working even if the flaws are present.
// To uncover these flaws, we add the "assert" statement.
// One potential future flaw is that the bibEntry is modified concurrently and the field being deleted.
Optional<String> resolvedFieldLatexFree = bibEntry.getResolvedFieldOrAliasLatexFree(field, this.databaseContext.getDatabase());
assert resolvedFieldLatexFree.isPresent();
addBatch(preparedStatement, entryId, field, value, resolvedFieldLatexFree.orElse(""));

// Skip indexing of date-related fields separately to ensure proper handling later in the process.
if (!DATE_FIELDS.contains(field)) {
Optional<String> resolvedFieldLatexFree = bibEntry.getResolvedFieldOrAliasLatexFree(field, this.databaseContext.getDatabase());
assert resolvedFieldLatexFree.isPresent();
addBatch(preparedStatement, entryId, field, value, resolvedFieldLatexFree.orElse(""));
}
Comment on lines +221 to +225
Copy link

Choose a reason for hiding this comment

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

The code uses assert and orElse("") for Optional handling instead of proper Optional methods. Should use ifPresent() for better Optional handling.

Copy link
Member

Choose a reason for hiding this comment

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

You can use resolvedFieldLatexFree.ifPresent(content -> ...)

Copy link
Contributor Author

@AbdoMostfa2 AbdoMostfa2 Mar 21, 2025

Choose a reason for hiding this comment

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

I understand the point about using ifPresent() for better Optional handling. However, I deliberately retained the assertion because the comment emphasizes detecting potential flaws.
image

Copy link
Member

Choose a reason for hiding this comment

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

Do both.

Consistency with other code.

We cannot change current tooling easily.

Copy link
Member

Choose a reason for hiding this comment

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

You can also use get() and ignore the bot comment. Seems to be best (seeing these two lines only)

// region Handling of known multi-value fields
// split and convert to Unicode
if (field.getProperties().contains(FieldProperty.PERSON_NAMES)) {
Expand All @@ -239,7 +244,11 @@ private void addToIndex(BibEntry bibEntry) {
}
// endregion
}

// ensure all date-related fields are indexed.
for (Field dateField : DATE_FIELDS) {
Optional<String> resolvedDateValue = bibEntry.getResolvedFieldOrAlias(dateField, this.databaseContext.getDatabase());
resolvedDateValue.ifPresent(dateValue -> addBatch(preparedStatement, entryId, dateField, dateValue));
}
// add entry type
addBatch(preparedStatement, entryId, TYPE_HEADER, bibEntry.getType().getName());

Expand Down Expand Up @@ -302,16 +311,47 @@ private void insertField(BibEntry entry, Field field) {
FIELD_VALUE_LITERAL,
FIELD_VALUE_TRANSFORMED);

try (PreparedStatement preparedStatement = connection.prepareStatement(insertFieldQuery)) {
String entryId = entry.getId();
String value = entry.getField(field).orElse("");
// Inserts or updates date-related fields (e.g., date, year, month, day) into the index.
// If a conflict occurs (e.g., the same ENTRY_ID and FIELD_NAME already exist),
// the existing values are overwritten with the new ones to ensure the latest data is stored.
String insertDateFieldQuery = """
INSERT INTO %s ("%s", "%s", "%s", "%s")
VALUES (?, ?, ?, ?)
ON CONFLICT ("%s", "%s")
Copy link
Member

Choose a reason for hiding this comment

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

ON CONFLICT is not clear to me. Add Java coment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This handles situations where the user inserts conflicting data, such as date=2025-03-17 and year=2026. In this case, it will overwrite the previously calculated year. Should I ignore this conflict instead?

Copy link
Member

Choose a reason for hiding this comment

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

Please add Java comment

DO UPDATE SET "%s" = EXCLUDED."%s", "%s" = EXCLUDED."%s"
""".formatted(
schemaMainTableReference,
ENTRY_ID,
FIELD_NAME,
FIELD_VALUE_LITERAL,
FIELD_VALUE_TRANSFORMED,
ENTRY_ID, FIELD_NAME,
FIELD_VALUE_LITERAL, FIELD_VALUE_LITERAL,
FIELD_VALUE_TRANSFORMED, FIELD_VALUE_TRANSFORMED);

String entryId = entry.getId();
// If the updated field is date-related,iterate through all date fields and update the index accordingly.
if (DATE_FIELDS.contains(field)) {
try (PreparedStatement preparedStatement = connection.prepareStatement(insertDateFieldQuery)) {
for (Field dateField : DATE_FIELDS) {
Optional<String> resolvedDateValue = entry.getResolvedFieldOrAlias(dateField, this.databaseContext.getDatabase());
resolvedDateValue.ifPresent(dateValue -> addBatch(preparedStatement, entryId, dateField, dateValue));
}
preparedStatement.executeBatch();
} catch (SQLException e) {
LOGGER.error("Could not add an entry to the index.", e);
}
} else {
try (PreparedStatement preparedStatement = connection.prepareStatement(insertFieldQuery)) {
String value = entry.getField(field).orElse("");

Optional<String> resolvedFieldLatexFree = entry.getResolvedFieldOrAliasLatexFree(field, this.databaseContext.getDatabase());
assert resolvedFieldLatexFree.isPresent();
addBatch(preparedStatement, entryId, field, value, resolvedFieldLatexFree.orElse(""));
preparedStatement.executeBatch();
} catch (SQLException e) {
LOGGER.error("Could not add an entry to the index.", e);
Optional<String> resolvedFieldLatexFree = entry.getResolvedFieldOrAliasLatexFree(field, this.databaseContext.getDatabase());
assert resolvedFieldLatexFree.isPresent();
addBatch(preparedStatement, entryId, field, value, resolvedFieldLatexFree.orElse(""));
preparedStatement.executeBatch();
} catch (SQLException e) {
LOGGER.error("Could not add an entry to the index.", e);
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Why empty line removed?

String insertIntoSplitTable = """
Expand All @@ -325,7 +365,6 @@ private void insertField(BibEntry entry, Field field) {
FIELD_VALUE_TRANSFORMED);

try (PreparedStatement preparedStatement = connection.prepareStatement(insertIntoSplitTable)) {
String entryId = entry.getId();
Copy link
Member

Choose a reason for hiding this comment

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

I don't know why this part of the code is touched. I totally forgot about the use of the split fields. I asked @LoayGhreeb for support at documentaiotn

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 think the Split Table handles multi-valued fields like keywords or groups. I did not remove it, but the entryId is calculated above.

Copy link
Member

Choose a reason for hiding this comment

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

Could you add a Java comment to org.jabref.model.search.PostgreConstants#getSplitTableSchemaReference please?

String value = entry.getField(field).orElse("");

if (field.getProperties().contains(FieldProperty.PERSON_NAMES)) {
Expand Down
4 changes: 4 additions & 0 deletions src/main/java/org/jabref/model/search/PostgreConstants.java
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ public static String getMainTableSchemaReference(String mainTable) {
return BIB_FIELDS_SCHEME + ".\"" + mainTable + "\"";
}

/**
* Generates the schema reference for the split table, which is used to store multi-value fields (e.g., authors, keywords)
* in a normalized form to facilitate efficient querying and indexing of the data.
*/
public static String getSplitTableSchemaReference(String mainTable) {
return BIB_FIELDS_SCHEME + ".\"" + mainTable + SPLIT_TABLE_SUFFIX + "\"";
}
Expand Down
Loading