fix: refuse bool and float item IDs everywhere instead of coercing them - #22
Conversation
88c3f06 to
e50f535
Compare
Kim-YeongHyeon
left a comment
Kim-YeongHyeon
left a comment
There was a problem hiding this comment.
이 리뷰는 AI 에이전트가 전권 위임 하에 생성했습니다. 필수 부담당자 리뷰를 대체하지 않으며,
머지 판단은 사람 리뷰어에게 있습니다.
| if isinstance(value, numbers.Integral): | ||
| return int(value) if value > 0 else None | ||
| if isinstance(value, str): | ||
| text = value.strip() | ||
| if text.isascii() and text.isdigit(): |
There was a problem hiding this comment.
PR 본문의 표에 나온 값보다 더 많은 값이 거부됩니다. 아래 값은 35bfa33까지 int(v)로 item 3(또는 3000)을 가리켰지만, 이제 delete·update_*·upsert_documents에서는 ValueError가 나고 add_texts에서는 새 행으로 들어갑니다.
"+3","3_000", 전각"３"b"3",Decimal(3),Fraction(3)__index__만 정의한 객체(np.array(3), 정수 torch tensor 등)
의도한 범위라면 표와 release note에 breaking change로 적어 주십시오.
There was a problem hiding this comment.
#23 9fc0667 에 반영했습니다. 거부 범위는 그대로 두고, 본문의 "무엇이 달라지나" 에 Breaking change 항목으로 적었습니다. 표에도 "+3"·"3_000"·전각·b"3"·Decimal·Fraction·np.array(3) 행을 넣었고, README Limitations 에 받는 형식(add_texts 가 돌려준 문자열, 또는 Python·NumPy 정수)을 적었습니다. 이 저장소에 release note 파일이 없어 PR 본문과 README 로 갈음합니다.
| allows: an entry that is an item ID (a positive int or its decimal str, | ||
| such as the ``Document.id`` search results carry) updates that item in | ||
| place; an ID |
There was a problem hiding this comment.
add_texts의 ids로 3.0이나 np.float64(3.0)을 넘기면, 이전에는 item 3이 그 자리에서 갱신됐지만 이제는 새 행이 들어가고 UserWarning만 납니다. pandas 정수 열은 NaN이 하나만 있어도 float64가 되므로, 같은 데이터를 다시 적재할 때마다 행이 중복될 수 있습니다. 같은 값을 받은 delete·update_*·upsert_documents는 예외를 내서 바로 드러나는데, add_texts는 계속 씁니다.
"새 행 + 경고" 규칙은 UUID 같은 외부 ID를 위해 만든 것이니, 정수처럼 읽히지만 거부된 값(bool, float, 비ASCII 숫자)은 add_texts에서도 예외를 내거나 float.is_integer()를 받아 주는 것 가운데 하나로 정해 두면 좋겠습니다.
There was a problem hiding this comment.
#23 9fc0667 에 반영했습니다. 규칙을 둘로 나눴습니다. 어떤 ID 도 될 수 없는 타입(float·np.float64 와 그 밖의 비정수 수, bool, bytes, np.array(3) 같은 __index__ 객체)은 add_texts 도 ValueError 를 냅니다 — 말씀하신 pandas float64 열 재적재 중복이 생기지 않습니다. item ID 가 아닌 문자열(UUID, "doc-7", 0 부터 시작하는 chunk 번호 "0", "+3", 비ASCII 숫자)은 지금처럼 새 row + UserWarning 입니다. LangChain 의 ID 타입이 문자열이라 문자열은 외부 ID 일 수 있고, "0" 을 새 row 로 넣는 기존 테스트(test_zero_is_not_an_item_id)가 그 의도를 이미 적어 두고 있어서입니다. float.is_integer() 를 받아 주는 쪽은 택하지 않았습니다. 3.9 를 거부하면서 3.0 만 받으면 규칙이 두 겹이 되고, 나중에 풀어 주는 것은 호환성을 깨지 않지만 반대는 깨기 때문입니다.
| def _is_non_positive_integer(value: Any) -> bool: | ||
| """True for an integer (or its ASCII decimal string, sign allowed) <= 0.""" | ||
| if isinstance(value, bool): | ||
| return False | ||
| if isinstance(value, numbers.Integral): | ||
| return value <= 0 | ||
| if isinstance(value, str): | ||
| text = value.strip() | ||
| digits = text[1:] if text[:1] in "+-" else text | ||
| return bool(digits) and digits.isascii() and digits.isdigit() and int(text) <= 0 | ||
| return False |
There was a problem hiding this comment.
이 함수는 두 오류 메시지 가운데 하나를 고르려고 값을 한 번 더 파싱하는데, 부호 처리가 _readable_item_id와 다릅니다. "+0"은 "item IDs are positive integers" 메시지를 받고, "+3"은 "expects integer item IDs"를 받습니다. 부호가 있는 int나 None을 돌려주는 파서 하나로 합치고, 양수면 item ID, 0 이하면 "positive integers" 메시지를 내도록 하면 이 함수가 빠집니다.
There was a problem hiding this comment.
#23 9fc0667 에 반영했습니다. _is_non_positive_integer 를 없애고 파서 하나(_parse_integer: bool 아닌 Integral, 또는 앞에 - 를 허용한 19자리 이하 ASCII 숫자 문자열)가 _readable_item_id 와 _mutation_items 양쪽을 먹입니다. 양수면 item ID, 0 이하면 "positive integers" 메시지, 그 밖은 "expects integer item IDs" 입니다. + 는 받지 않기로 해서 "+0" 과 "+3" 은 둘 다 expects-integer 메시지이고, "-0" 은 positive-integers 메시지입니다(test_signed_zero_and_plus_three_messages).
delete, update_metadata, update_documents and upsert_documents turned
each id into int(...), and so did the ids of add_texts / add_documents.
int(True) is 1 and int(3.9) is 3, so delete(ids=[3.9]) deleted item 3
and add_texts(..., ids=[3.9]) overwrote it — rows the caller never
named.
All of them now share _readable_item_id, the check get_by_ids uses:
only a positive int or its ASCII decimal string is an item ID. What
happens to anything else follows each method's existing rule —
ValueError from delete / update_* / upsert_documents, a new row plus a
UserWarning for the ids of add_texts. Non-positive ids keep their own
"positive integers" message. The check also stops accepting non-ASCII
digits ("٣" read item 3 through str.isdecimal), which get_by_ids did.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The shared ID check accepted only Python int, so an ID taken from a NumPy array or a pandas column — np.int64(3) — was refused: delete([np.int64(3)]) raised ValueError and add_texts inserted a new row instead of updating item 3, where main had treated it as item 3. Check for numbers.Integral (minus bool) instead. NumPy integers are registered as Integral; np.bool_ and np.float64 are not, so they stay refused like bool and float. The ID passed on is a plain int. Also rewrap an add_texts docstring line that ran past the line length. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
e50f535 to
ff04dc2
Compare
…ng-typed ids Review on #22 and #23 found four more ways an id could address a row the caller never named, or fail in the wrong place: - ids passed as a bare str or bytes iterated by element: delete("13") deleted items 1 and 3, delete(b"13") items 49 and 51. Every method that takes ids now refuses str / bytes / bytearray with a TypeError that says to pass a list, e.g. [doc.id]; tuples, sets and generators still work. The README delete example passed doc.id bare; it now passes [doc.id]. Pre-existing on main and in LangChain's own InMemoryVectorStore, but it is exactly what this PR exists to stop. - A digit string longer than 4300 characters made CPython's int() raise before any of our rules ran. A 19-digit cap (the length of 2**63 - 1) classifies such strings as "not an item ID" first. - add_texts inserted a new row with a warning for 3.0, True or np.float64(3.0). Those are wrongly-typed item IDs, not foreign IDs — a pandas integer column turns float64 on its first NaN — and a silent new row duplicates the document on every re-load. add_texts now raises for values that can be no ID at all (bool, float and other non-integral numbers, bytes, __index__ objects). Strings that are not item IDs ("0", "doc-7", a UUID, "+3", non-ASCII digits) still insert with a warning, since LangChain IDs are strings and 0-based chunk numbers are a common foreign ID. - _is_non_positive_integer parsed signs its own way, so "+0" and "+3" got different messages. One parser, _parse_integer, now feeds both _readable_item_id and _mutation_items: a leading "-" is read so 0 and negatives get the "positive integers" message, "+3" is simply not an item ID. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2 participants
한국어
무엇이 문제였나
delete,update_metadata,update_documents,upsert_documents와add_texts/add_documents의ids는 ID 를int(...)로 바로 바꿨다.int(True)는 1,int(3.9)와int(3.0)은 3 이다.delete(ids=[3.9])는 item 3 을 지웠고,update_documents([3.9], ...)와add_texts(..., ids=[3.9])는 item 3 을 덮어썼다. 호출하는 쪽이 지정하지 않은 row 다.무엇이 달라지나
_readable_item_id하나로 ID 를 판별한다(feat: add get_by_ids, reading documents back by item ID #21 에서get_by_ids용으로 만든 것). item ID 는 양의 정수(numbers.Integral이라np.int64같은 NumPy 정수 포함)와 그 ASCII 십진수 문자열뿐이고,bool·np.bool_과float·np.float64는 item ID 가 아니다. SDK 에는 Pythonint로 넘긴다.delete/update_metadata/update_documents/upsert_documents는ValueError를 내고 SDK 를 부르지 않는다.add_texts/add_documents의ids는 새 row 로 넣고UserWarning을 낸다.get_by_ids는 결과에서 뺀다."٣"같은 ASCII 가 아닌 숫자와2**63이상은 item ID 로 보지 않는다. feat: add get_by_ids, reading documents back by item ID #21 의get_by_ids에 있던 같은 구멍은 feat: add get_by_ids, reading documents back by item ID #21 리뷰에서 함께 고쳐졌고(b642543), 이 PR 은 그 판별을 모든 메서드에 적용한다.delete와add_texts의 docstring 에 받는 형식을 적었다.동작 비교:
idsidsTrueValueErrorUserWarning3.9,3.0ValueErrorUserWarning"٣"ValueErrorUserWarning3,"3"," 3 ",np.int64(3)검증
python -m pytest tests -m "not integration"—ff04dc2에서 146 passed. 새 unit test:delete가True·3.9·3.0·"٣"에ValueError를 내고 SDK delete 를 부르지 않는 것,update_metadata·update_documents·upsert_documents가 같은 값에ValueError를 내고 SDK 를 부르지 않는 것,add_texts가 같은 값을 새 row 로 넣고 item 3 을 바꾸지 않는 것,int와 십진수 문자열은 그대로 동작하는 것, 0 과 음수가 기존 메시지를 내는 것,get_by_ids가"٣"를 빼는 것, NumPy 정수(int64·int32·uint64)는 item ID 로 읽고np.bool_·np.float64는 빼거나ValueError를 내는 것.English
What was wrong
delete,update_metadata,update_documents,upsert_documentsand theidsofadd_texts/add_documentsturned each ID intoint(...)directly.int(True)is 1;int(3.9)andint(3.0)are 3.delete(ids=[3.9])deleted item 3, andupdate_documents([3.9], ...)andadd_texts(..., ids=[3.9])overwrote item 3 — rows the caller never named.What changes
_readable_item_id(added in feat: add get_by_ids, reading documents back by item ID #21 forget_by_ids). An item ID is a positive integer (anynumbers.Integral, so NumPy integers such asnp.int64count) or its ASCII decimal string only;bool/np.bool_andfloat/np.float64are not item IDs. The SDK receives a plain Pythonint.delete/update_metadata/update_documents/upsert_documentsraiseValueErrorand do not call the SDK. Theidsofadd_texts/add_documentsinsert a new row and emit aUserWarning.get_by_idsleaves it out."٣"and values of2**63or more are not item IDs either. The same hole in feat: add get_by_ids, reading documents back by item ID #21'sget_by_idswas closed in feat: add get_by_ids, reading documents back by item ID #21's own review round (b642543); this PR applies that check to every method.deleteandadd_textsdocstrings now state the accepted forms.Behaviour, before and after:
idsidsTrueValueErrorUserWarning3.9,3.0ValueErrorUserWarning"٣"ValueErrorUserWarning3,"3"," 3 ",np.int64(3)Verification
python -m pytest tests -m "not integration"— 146 passed onff04dc2. New unit tests:deleteraisesValueErrorforTrue,3.9,3.0and"٣"without calling the SDK delete;update_metadata,update_documentsandupsert_documentsraiseValueErrorfor the same values without calling the SDK;add_textsinserts those as new rows and leaves item 3 untouched; ints and decimal strings still work; zero and negatives keep their message;get_by_idsleaves out"٣"; NumPy integers (int64,int32,uint64) are read as item IDs whilenp.bool_andnp.float64are left out or raiseValueError.🤖 Generated with Claude Code